Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support InstrumentationScope, and update OTLP proto to 0.18.0 #1345

Conversation

ahayworth
Copy link
Contributor

@ahayworth ahayworth commented Jul 14, 2022

This commit accomplishes two things simultaneously - adding support for
InstrumentationScope everywhere, and also updating the OTLP proto to v0.18.0.

The PR that introduced InstrumentationScope is here

My best understanding (which is implemented in this PR) is that:

  • Our OTLP export requests should group by InstrumentationScope instead
    of InstrumentationLibrary
  • We must be able to support accessing the InstrumentationLibrary from
    anywhere a ReadableSpan is available, and that it should represent the
    name and version fields from the InstrumentationScope.
  • When creating a tracer, we create and store an InstrumentationScope
    rather than an InstrumentationLibrary.
  • Non-OTLP exporters must export both otlp.scope.{name,version} AND
    otlp.library.{name,version} tags for backwards compatibility.

Some notes that may be interesting:

  • I chose to keep the original definition of InstrumentationLibrary
    around for now.
  • I chose to have Span#instrumentation_library and
    SpanData#instrumentation_library create and memoize an InstrumentationLibrary
    on-demand when someone asks for it (and marked the method as
    deprecated in the YARD docs).
  • I chose not to reference that deprecated helper when modifying the
    zipkin and jaeger exporters, for performance reasons.

cc @robbkidd - This PR does both OTLP proto updates and InstrumentationLibrary changes, as per your request. Can you verify that this looks right to you?
cc @robertlaurin - Is the metrics side of this looking okay?
cc @fbogsany - This change isn't actually huge in absolute terms, but getting it wrong would be catastrophic. I'd love your review on it.

edit: also worth mentioning that I really didn't see anything relevant to tracing in the proto changes. Metrics? Tons, definitely, but since we don't have any metrics exporters that seems okay for now.

Fixes #1126.

This commit accomplishes two things simultaneously - adding support for
InstrumentationScope everywhere, and also updating the OTLP proto to v0.18.0.

The PR that introduced InstrumentationScope is [here](open-telemetry/opentelemetry-specification#2276)

My best understanding (which is implemented in this PR) is that:
- Our OTLP export requests should group by InstrumentationScope instead
  of InstrumentationLibrary
- We must be able to support accessing the InstrumentationLibrary from
  anywhere a ReadableSpan is available, and that it should represent the
  `name` and `version` fields from the InstrumentationScope.
- When creating a tracer, we create and store an InstrumentationScope
  rather than an InstrumentationLibrary.
- Non-OTLP exporters must export both `otlp.scope.{name,version}` AND
  `otlp.library.{name,version}` tags for backwards compatibility.

Some notes that may be interesting:
- I chose to keep the original definition of `InstrumentationLibrary`
  around for now.
- I chose to have `Span#instrumentation_library` and
  `SpanData#instrumentation_library` create and memoize an `InstrumentationLibrary`
  on-demand when someone asks for it (and marked the method as
  deprecated in the YARD docs).
- I chose *not* to reference that deprecated helper when modifying the
  zipkin and jaeger exporters, for performance reasons.
@ahayworth ahayworth force-pushed the ahayworth/instrumentation-scope-and-proto-bump branch from a92cc56 to e525c4e Compare July 14, 2022 22:34
@fbogsany
Copy link
Contributor

All the gems that access instrumentation_scope from the span data object will need to update their SDK dependency to a minimum of the SDK version that releases this change. We can only do that during the release PR, but we have to ensure the version dependencies are correct at that time.

@fbogsany
Copy link
Contributor

We should add a deprecation notice to the InstrumentationLibrary struct documentation.

@ahayworth ahayworth requested a review from robbkidd as a code owner July 15, 2022 18:49
@ahayworth ahayworth requested a review from fbogsany July 15, 2022 19:26
Copy link
Contributor

@fbogsany fbogsany left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bunch of small documentation suggestions. Otherwise LGTM. Thanks!

Copy link
Contributor

@robertlaurin robertlaurin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you update the rake file for updating the protos? https://github.com/open-telemetry/opentelemetry-ruby/blob/main/exporter/otlp/Rakefile#L32-L37

They seem to be still excluding metrics stuff.

Or did you update them via some other approach?

@ahayworth
Copy link
Contributor Author

Did you update the rake file for updating the protos? https://github.com/open-telemetry/opentelemetry-ruby/blob/main/exporter/otlp/Rakefile#L32-L37

Oh - I did it manually by 1) cloning the proto repo and switching to the tag, 2) running make gen-ruby, and 3) copying the generated files to the right locations.

I updated the rake task (and it now checks out a specific tag rather than whatever is on main, and generates a bunch more protos) in c3e3754, and I re-generated in bfd7122 (which is just a bunch of whitespace changes).

@robertlaurin
Copy link
Contributor

This is good to merge, but I want to wait for @robbkidd to chime in because I want to make sure I/we are remembering correctly that there was a friendly request for these to get bumped in tandem.

@fbogsany
Copy link
Contributor

fbogsany commented Aug 4, 2022

Can we push this over the line @robbkidd , or is some synchronization required?

@robbkidd
Copy link
Member

robbkidd commented Aug 9, 2022

Sorry for my silence, folks. Yes, I will review this today!

@robbkidd robbkidd self-assigned this Aug 9, 2022
Copy link
Member

@robbkidd robbkidd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the changes and ran the demo webstore emailservice with the SDK and OTLP exporter tracking this branch.

Deets on gem swap out in webstore emailservice
  • Deleted src/emailservice/Gemfile.lock and made the following changes:
diff --git a/src/emailservice/Dockerfile b/src/emailservice/Dockerfile
index 1b6d1d8..df8ba14 100644
--- a/src/emailservice/Dockerfile
+++ b/src/emailservice/Dockerfile
@@ -14,14 +14,14 @@

 FROM ruby:3.1.2-slim

-RUN apt-get update -y && apt-get install -y build-essential
+RUN apt-get update -y && apt-get install -y build-essential git
+
+WORKDIR /email_server

 COPY Gemfile* ./

 RUN bundle install

-WORKDIR /email_server
-
 COPY . .

 EXPOSE ${EMAIL_SERVICE_PORT}
diff --git a/src/emailservice/Gemfile b/src/emailservice/Gemfile
index 971b0d0..77f2408 100644
--- a/src/emailservice/Gemfile
+++ b/src/emailservice/Gemfile
@@ -5,6 +5,6 @@ gem "pony",     "~> 1.13"
 gem "puma",     "~> 5.6"
 gem "sinatra",  "~> 2.2"

-gem "opentelemetry-sdk",                     "~> 1.1"
-gem "opentelemetry-exporter-otlp",           "~> 0.21"
+gem "opentelemetry-sdk",                     github: 'ahayworth/opentelemetry-ruby', ref: 'ahayworth/instrumentation-scope-and-proto-bump', glob: 'sdk/*.gemspec'
+gem "opentelemetry-exporter-otlp",           github: 'ahayworth/opentelemetry-ruby', ref: 'ahayworth/instrumentation-scope-and-proto-bump', glob: 'exporter/otlp/*.gemspec'
 gem "opentelemetry-instrumentation-sinatra", "~> 0.19"
  • ✅ received trace spans from the Ruby service with no code changes to the service
  • ✅ library.name span attribute (and, really, any span data at all) remains populated in a receiver using an older proto version

Screen Shot 2022-08-09 at 17 51 12

@ahayworth
Copy link
Contributor Author

That is a far more full-featured test than I was expecting, thank you @robbkidd !

@robbkidd
Copy link
Member

That is a far more full-featured test than I was expecting

Making up for the hang-time!

@fbogsany
Copy link
Contributor

Good to merge once the conflict is resolved.

@robbkidd robbkidd removed their assignment Aug 10, 2022
@ahayworth
Copy link
Contributor Author

@fbogsany I merged main and the conflict went away ... which seems mysterious but I do think the git client has gotten smarter about stuff these days. 🤷

@robertlaurin robertlaurin merged commit 03e36ce into open-telemetry:main Aug 22, 2022
@sodabrew
Copy link

This is awesome! Will there be a release to Rubygems soon?

@ahayworth ahayworth deleted the ahayworth/instrumentation-scope-and-proto-bump branch August 24, 2022 12:03
@ahayworth
Copy link
Contributor Author

@sodabrew we could cut one - I will ask in the CNCF slack (#otel-ruby, if you wish to join).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

spec update: InstrumentationLibrary/InstrumentationScope
5 participants