Clarify that Trace/Meter are associated with Instrumentation Scope #2276

tigrannajaryan · 2022-01-19T22:36:04Z

This is a slightly different take on #2203

Instead of renaming instrumentation library to instrumentation scope everywhere
this PR suggests targetted editing of wording of the Trace/Meter obtaining API
to allow not just instrumentation library but other instrumentation scopes to be
used as a parameter.

This change does not force renaming of API parameters and is not a breaking change.
We consider it a clarification of usage to match the intent that we had for the "name"
field.

If this PR is accepted there will be a follow up PR that will suggest to rename
InstrumentationLibrary* messages in OTLP proto to InstrumentationScope* message.
Such a change will not be a breaking change for the OTLP wire format and is acceptable.

If this PR is accepted we will also close #1236
since it will be no longer necessary. The logger name will be recorded as the
instrumentation scope, done in a separate PR that clarifies the behavior in https://github.com/open-telemetry/oteps/blob/main/text/logs/0150-logging-library-sdk.md

Resolves #2307

specification/glossary.md

carlosalberto · 2022-01-20T17:55:09Z

Overall LGTM - I'd like to get the opinion of maintainers though.

cc @tedsuo

tigrannajaryan · 2022-01-21T14:47:09Z

Let's keep this open for at least one week. I want to make sure lots of folks see this.

tigrannajaryan · 2022-01-21T14:47:51Z

We should also mention this in the Maintainers and Spec SIG meetings next week.

bogdandrutu · 2022-01-21T18:47:42Z

@tigrannajaryan I created an entry for the next week maintainer agenda meeting, and add this as a discussion point. I will not be there.

tigrannajaryan · 2022-01-24T15:27:35Z

@open-telemetry/specs-approvers I won't be able to attend the Maintainers meeting today. If you are in the meeting please ask maintainers to have a look / be aware of this change.

specification/metrics/api.md

specification/trace/api.md

specification/metrics/api.md

specification/trace/api.md

tigrannajaryan · 2022-01-25T14:15:07Z

@open-telemetry/specs-metrics-approvers @open-telemetry/specs-trace-approvers please review.

dyladan

Expanding the definition of a field that was already declared stable such that it is now possible to lose the originally intended information contained in the field (by using a class name instead of the actual library name) breaks the field.

This is breaking for receivers who depend on the instrumentation library name being contained in this field, with no way to recover this information
Logger name will not always refer to the instrumentation library as this field was intended. Actually, it is extremely unlikely to refer to the instrumentation library and would rather refer to the thing being instrumented.

tigrannajaryan · 2022-01-25T15:52:32Z

@dyladan to clarify, your position is that we should not make this change for Tracer/Meter name because it is a breaking change. You think that it is OK to assign a different meaning to the Logger name field, i.e. accept that it is not going to be consistent with Tracer/Meter name in its semantics.

Contributes to open-telemetry#1215 Contributes to open-telemetry#2358 The topics has been discussed at length. This [PR](open-telemetry#2276) made possible to record instrumentation scope for traces and metrics. The intent is also to have instrumentation scope part of log data model and where available record logger name as the instrumentation scope name.

Contributes to #1215 Contributes to #2358 The topics has been discussed at length. This [PR](#2276) made possible to record instrumentation scope for traces and metrics. The intent is also to have instrumentation scope part of log data model and where available record logger name as the instrumentation scope name.

This applies changes described in spec PR open-telemetry/opentelemetry-specification#2276 This is not a breaking change for OTLP. The wire format does not change. The change is done in a way that the transition can be handled gracefully for OTLP/JSON and for the generated code. See the comments for explanation on how to do it in the senders and receivers.

Spec change can be found in PR: open-telemetry/opentelemetry-specification#2276

OTEL-PHP at some point switched to using InstrumentationScope instead of InstrumentationLibrary everywhere, which incidentally satisfied the ["Fetch InstrumentationScope from ReadableSpan"](https://github.com/open-telemetry/opentelemetry-specification/blob/9abbdd39d0b35f635f833f072013431da419894e/specification/common/mapping-to-non-otlp.md#instrumentationscope) part of the spec (otel-php is still alpha so it seemed ok to not deprecate the instrumentation library and just remove it's use entirely) Related issues # open-telemetry/opentelemetry-php#705 Related [OTEP(s)](https://github.com/open-telemetry/oteps) # Don't think it was an OTEP but #2276 is where the push for the change originally came from I believe

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.

…1345) * 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](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. * Update sdk/test/opentelemetry/sdk/trace/span_test.rb Co-authored-by: Francis Bogsanyi <francis.bogsanyi@shopify.com> * Update sdk/lib/opentelemetry/sdk/trace/span_data.rb Co-authored-by: Francis Bogsanyi <francis.bogsanyi@shopify.com> * Update sdk/lib/opentelemetry/sdk/trace/span.rb Co-authored-by: Francis Bogsanyi <francis.bogsanyi@shopify.com> * fixup: use alias_method * fixup: add deprecation notice to InstrumentationLibrary * fixup: argh, use alias in class scope * Update sdk/lib/opentelemetry/sdk/trace/span.rb Co-authored-by: Francis Bogsanyi <francis.bogsanyi@shopify.com> * Update sdk/lib/opentelemetry/sdk/trace/span.rb Co-authored-by: Francis Bogsanyi <francis.bogsanyi@shopify.com> * Update sdk/lib/opentelemetry/sdk/trace/span_data.rb Co-authored-by: Francis Bogsanyi <francis.bogsanyi@shopify.com> * fixup: update Rakefile to regenerate protos correctly It now checks out a specific tag, for starters. * bundle exec rake update_protobuf Co-authored-by: Francis Bogsanyi <francis.bogsanyi@shopify.com>

…metry#2622) OTEL-PHP at some point switched to using InstrumentationScope instead of InstrumentationLibrary everywhere, which incidentally satisfied the ["Fetch InstrumentationScope from ReadableSpan"](https://github.com/open-telemetry/opentelemetry-specification/blob/9abbdd39d0b35f635f833f072013431da419894e/specification/common/mapping-to-non-otlp.md#instrumentationscope) part of the spec (otel-php is still alpha so it seemed ok to not deprecate the instrumentation library and just remove it's use entirely) Related issues # open-telemetry/opentelemetry-php#705 Related [OTEP(s)](https://github.com/open-telemetry/oteps) # Don't think it was an OTEP but open-telemetry#2276 is where the push for the change originally came from I believe

Contributes to open-telemetry/opentelemetry-specification#1215 Contributes to open-telemetry/opentelemetry-specification#2358 The topics has been discussed at length. This [PR](open-telemetry/opentelemetry-specification#2276) made possible to record instrumentation scope for traces and metrics. The intent is also to have instrumentation scope part of log data model and where available record logger name as the instrumentation scope name.

OTEL-PHP at some point switched to using InstrumentationScope instead of InstrumentationLibrary everywhere, which incidentally satisfied the ["Fetch InstrumentationScope from ReadableSpan"](https://github.com/open-telemetry/opentelemetry-specification/blob/65624332cadac9990923f442e96a424c308e502c/specification/common/mapping-to-non-otlp.md#instrumentationscope) part of the spec (otel-php is still alpha so it seemed ok to not deprecate the instrumentation library and just remove it's use entirely) Related issues # open-telemetry/opentelemetry-php#705 Related [OTEP(s)](https://github.com/open-telemetry/oteps) # Don't think it was an OTEP but open-telemetry/opentelemetry-specification#2276 is where the push for the change originally came from I believe

Contributes to open-telemetry/opentelemetry-specification#1215 Contributes to open-telemetry/opentelemetry-specification#2358 The topics has been discussed at length. This [PR](open-telemetry/opentelemetry-specification#2276) made possible to record instrumentation scope for traces and metrics. The intent is also to have instrumentation scope part of log data model and where available record logger name as the instrumentation scope name.

OTEL-PHP at some point switched to using InstrumentationScope instead of InstrumentationLibrary everywhere, which incidentally satisfied the ["Fetch InstrumentationScope from ReadableSpan"](https://github.com/open-telemetry/opentelemetry-specification/blob/9abbdd39d0b35f635f833f072013431da419894e/specification/common/mapping-to-non-otlp.md#instrumentationscope) part of the spec (otel-php is still alpha so it seemed ok to not deprecate the instrumentation library and just remove it's use entirely) Related issues # open-telemetry/opentelemetry-php#705 Related [OTEP(s)](https://github.com/open-telemetry/oteps) # Don't think it was an OTEP but open-telemetry/opentelemetry-specification#2276 is where the push for the change originally came from I believe

…pen-telemetry#2276) * Clarify that Trace/Meter are associated with Instrumentation Scope This is a slightly different take on open-telemetry#2203 Instead of renaming instrumentation library to instrumentation scope everywhere this PR suggests targetted editing of wording of the Trace/Meter obtaining API to allow not just instrumentation library but other instrumentation scopes to be used as a parameter. This change does not force renaming of API parameters and is not a breaking change. We consider it a clarification of usage to match the intent that we had for the "name" field. If this PR is accepted there will be a follow up PR that will suggest to rename InstrumentationLibrary* messages in OTLP proto to InstrumentationScope* message. Such a change will not be a breaking change for the OTLP wire format and is acceptable. If this PR is accepted we will also close open-telemetry#1236 since it will be no longer necessary. The logger name will be recorded as the instrumentation scope. This clarification will be a follow up PR that clarifies the behavior in https://github.com/open-telemetry/oteps/blob/main/text/logs/0150-logging-library-sdk.md

tigrannajaryan requested review from a team January 19, 2022 22:36

github-actions bot assigned carlosalberto Jan 19, 2022

tigrannajaryan force-pushed the normalize-loggername branch from de293ee to 946afab Compare January 19, 2022 22:36

tigrannajaryan requested a review from a team January 19, 2022 22:39

tigrannajaryan added the area:api Cross language API specification issue label Jan 19, 2022

bogdandrutu approved these changes Jan 19, 2022

View reviewed changes

yurishkuro previously approved these changes Jan 20, 2022

View reviewed changes

GraemeF reviewed Jan 20, 2022

View reviewed changes

specification/glossary.md Outdated Show resolved Hide resolved

carlosalberto approved these changes Jan 20, 2022

View reviewed changes

MrAlias approved these changes Jan 20, 2022

View reviewed changes

jmacd approved these changes Jan 21, 2022

View reviewed changes

carlosalberto mentioned this pull request Jan 21, 2022

Specify the treatment of duplicate instruments and observations #2270

Closed

jack-berg approved these changes Jan 21, 2022

View reviewed changes

arminru reviewed Jan 24, 2022

View reviewed changes

specification/metrics/api.md Outdated Show resolved Hide resolved

specification/trace/api.md Outdated Show resolved Hide resolved

djaglowski approved these changes Jan 24, 2022

View reviewed changes

arminru added spec:metrics Related to the specification/metrics directory spec:trace Related to the specification/trace directory labels Jan 24, 2022

arminru reviewed Jan 24, 2022

View reviewed changes

specification/metrics/api.md Outdated Show resolved Hide resolved

dyladan reviewed Jan 25, 2022

View reviewed changes

specification/metrics/api.md Outdated Show resolved Hide resolved

specification/trace/api.md Outdated Show resolved Hide resolved

dyladan requested changes Jan 25, 2022

View reviewed changes

tigrannajaryan mentioned this pull request Feb 22, 2022

Rename InstrumentationLibrary to InstrumentationScope open-telemetry/opentelemetry-proto#362

Merged

srikanthccv mentioned this pull request Mar 9, 2022

Deprecate InstrumentationLibraryInfo open-telemetry/opentelemetry-python#2516

Closed

dashpole mentioned this pull request Mar 10, 2022

Add span attributes for library name and version GoogleCloudPlatform/opentelemetry-operations-go#332

Closed

srikanthccv mentioned this pull request Apr 5, 2022

Add InstrumentationScope and deprecate InstrumentationLibraryInfo open-telemetry/opentelemetry-python#2583

Merged

lalitb mentioned this pull request May 7, 2022

Add InstrumentationScope and deprecate InstrumentationLibrary open-telemetry/opentelemetry-cpp#1381

Closed

ahayworth mentioned this pull request May 11, 2022

spec update: InstrumentationLibrary/InstrumentationScope open-telemetry/opentelemetry-ruby#1126

Closed

MadVikingGod mentioned this pull request Jun 3, 2022

Move to using Instrumentation Scope open-telemetry/opentelemetry-go#2941

Closed

tsloughter pushed a commit to tsloughter/opentelemetry-erlang that referenced this pull request Jun 8, 2022

replace InstrumentationLibrary with InstrumentationScope

2569f17

Spec change can be found in PR: open-telemetry/opentelemetry-specification#2276

tsloughter mentioned this pull request Jun 8, 2022

replace InstrumentationLibrary with InstrumentationScope open-telemetry/opentelemetry-erlang#405

Merged

tsloughter pushed a commit to tsloughter/opentelemetry-erlang that referenced this pull request Jun 8, 2022

replace InstrumentationLibrary with InstrumentationScope

597e0ee

Spec change can be found in PR: open-telemetry/opentelemetry-specification#2276

tsloughter pushed a commit to tsloughter/opentelemetry-erlang that referenced this pull request Jun 11, 2022

replace InstrumentationLibrary with InstrumentationScope

657d8ff

Spec change can be found in PR: open-telemetry/opentelemetry-specification#2276

This was referenced Jun 14, 2022

Fetch InstrumentationScope from ReadableSpan open-telemetry/opentelemetry-php#705

Closed

Check Fetch InstrumentationScope from ReadableSpan Box for PHP #2622

Merged

ahayworth mentioned this pull request Jul 14, 2022

feat: support InstrumentationScope, and update OTLP proto to 0.18.0 open-telemetry/opentelemetry-ruby#1345

Merged

lmolkova mentioned this pull request Nov 20, 2023

Conflicting/confusing requirements around InstrumentationScope attributes #3773

Open

tigrannajaryan mentioned this pull request Jan 15, 2024

spec matrix: remove duplicate mention of meter+scope association #3822

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clarify that Trace/Meter are associated with Instrumentation Scope #2276

Clarify that Trace/Meter are associated with Instrumentation Scope #2276

tigrannajaryan commented Jan 19, 2022 •

edited

Loading

carlosalberto commented Jan 20, 2022

tigrannajaryan commented Jan 21, 2022

tigrannajaryan commented Jan 21, 2022

bogdandrutu commented Jan 21, 2022

tigrannajaryan commented Jan 24, 2022

tigrannajaryan commented Jan 25, 2022

dyladan left a comment

tigrannajaryan commented Jan 25, 2022

Clarify that Trace/Meter are associated with Instrumentation Scope #2276

Clarify that Trace/Meter are associated with Instrumentation Scope #2276

Conversation

tigrannajaryan commented Jan 19, 2022 • edited Loading

carlosalberto commented Jan 20, 2022

tigrannajaryan commented Jan 21, 2022

tigrannajaryan commented Jan 21, 2022

bogdandrutu commented Jan 21, 2022

tigrannajaryan commented Jan 24, 2022

tigrannajaryan commented Jan 25, 2022

dyladan left a comment

Choose a reason for hiding this comment

tigrannajaryan commented Jan 25, 2022

tigrannajaryan commented Jan 19, 2022 •

edited

Loading