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

Fetch InstrumentationScope from ReadableSpan #705

Closed
bobstrecansky opened this issue Jun 7, 2022 · 5 comments
Closed

Fetch InstrumentationScope from ReadableSpan #705

bobstrecansky opened this issue Jun 7, 2022 · 5 comments
Labels
bite sized This is a small chunk of work (good for new or time limited contributors!) help wanted This issue is looking for someone to work on it

Comments

@bobstrecansky
Copy link
Collaborator

The spec compliance matrix defines Fetch InstrumentationScope from ReadableSpan

We need to implement this.

@bobstrecansky bobstrecansky added help wanted This issue is looking for someone to work on it bite sized This is a small chunk of work (good for new or time limited contributors!) release:required-for-ga labels Jun 7, 2022
@Grunet
Copy link
Contributor

Grunet commented Jun 14, 2022

@bobstrecansky it looks like @brettmc updated this for the Jaeger and Zipkin cases at some point per this search.

Is the concern that this needs to be added to other non-OTLP exporter cases in the repo?

Or maybe that the instrumentation library piece needs to be added back for backwards compatibility? (for what exactly I'm not really clear. Maybe like if Jaeger and Zipkin have been configured to look for the instrumentation library piece perhaps already, it's not gonna work with just sending the instrumentation scope piece? Also I find it somewhat hard to follow the conversation on the original PR that updated the spec)

Or maybe other things?

@bobstrecansky
Copy link
Collaborator Author

This issue is mostly a placeholder to ensure that all of that spec compliant matrix is completed. Looks like you've done a bunch of sleuthing; I believe this is done for us. That deprecated bit shouldn't matter for us, maybe? We should probably instrument those as well for consistency's sake.

@brettmc
Copy link
Collaborator

brettmc commented Jun 15, 2022

It looks like we can fetch the instrumentation scope from a span, so as Bob said we just needed to verify that we have implemented per spec, and you could submit a PR against the compliance matrix to complete this issue.

@Grunet
Copy link
Contributor

Grunet commented Jun 20, 2022

Spun up open-telemetry/opentelemetry-specification#2622 on the assumption that the breaking changes the other libraries have to worry about are about direct consumers relying on the old instrumentation library piece, not backends (from looking through a few more PRs of other language libraries linked to that main spec PR, erlang being the only one I've seen so far that did the straight swap without deprecating).

tigrannajaryan pushed a commit to open-telemetry/opentelemetry-specification that referenced this issue Jun 21, 2022
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
@Grunet
Copy link
Contributor

Grunet commented Jun 22, 2022

The comparison matrix has been updated so probably good to close this now

beeme1mr pushed a commit to beeme1mr/opentelemetry-specification that referenced this issue Aug 31, 2022
…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
ChengJinbao added a commit to ChengJinbao/opentelemetry-specification that referenced this issue Nov 16, 2022
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
joaopgrassi pushed a commit to dynatrace-oss-contrib/semantic-conventions that referenced this issue Mar 21, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bite sized This is a small chunk of work (good for new or time limited contributors!) help wanted This issue is looking for someone to work on it
Projects
None yet
Development

No branches or pull requests

3 participants