opentelemetry-ruby icon indicating copy to clipboard operation
opentelemetry-ruby copied to clipboard

feat: support InstrumentationScope, and update OTLP proto to 0.18.0

Open ahayworth opened this issue 2 years ago • 9 comments

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.

ahayworth avatar Jul 14 '22 22:07 ahayworth

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 avatar Jul 15 '22 16:07 fbogsany

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

fbogsany avatar Jul 15 '22 16:07 fbogsany

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 bfd7122cfc668504ad5d99fec5476e6cc6bbf18b (which is just a bunch of whitespace changes).

ahayworth avatar Jul 22 '22 20:07 ahayworth

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.

robertlaurin avatar Jul 25 '22 15:07 robertlaurin

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

fbogsany avatar Aug 04 '22 21:08 fbogsany

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

robbkidd avatar Aug 09 '22 15:08 robbkidd

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

ahayworth avatar Aug 09 '22 22:08 ahayworth

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

Making up for the hang-time!

robbkidd avatar Aug 10 '22 00:08 robbkidd

Good to merge once the conflict is resolved.

fbogsany avatar Aug 10 '22 14:08 fbogsany

@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. 🤷

ahayworth avatar Aug 14 '22 14:08 ahayworth

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

sodabrew avatar Aug 23 '22 18:08 sodabrew

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

ahayworth avatar Aug 24 '22 12:08 ahayworth