opentelemetry-ruby
opentelemetry-ruby copied to clipboard
feat: support InstrumentationScope, and update OTLP proto to 0.18.0
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
andversion
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}
ANDotlp.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
andSpanData#instrumentation_library
create and memoize anInstrumentationLibrary
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.
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.
We should add a deprecation notice to the InstrumentationLibrary
struct documentation.
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).
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.
Can we push this over the line @robbkidd , or is some synchronization required?
Sorry for my silence, folks. Yes, I will review this today!
That is a far more full-featured test than I was expecting, thank you @robbkidd !
That is a far more full-featured test than I was expecting
Making up for the hang-time!
Good to merge once the conflict is resolved.
@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. 🤷
This is awesome! Will there be a release to Rubygems soon?
@sodabrew we could cut one - I will ask in the CNCF slack (#otel-ruby
, if you wish to join).