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

Move SpanKey to internal package #4869

Merged
merged 2 commits into from
Dec 14, 2021

Conversation

mateuszrzeszutek
Copy link
Member

This is probably a class that we don't want to expose in instrumentation-api's public API.

@mateuszrzeszutek
Copy link
Member Author

For the record: this PR will break the bridging with the previous instrumentation-api versions; but since it's still alpha maybe breaking changes are okay.

@trask
Copy link
Member

trask commented Dec 10, 2021

@lmolkova does this cause any issues for your use case(s)?

@lmolkova
Copy link
Contributor

@trask thanks for the heads up! I can work around it.

I have a small concern around it: assuming I don't use instrumentation-api to instrument my library (which is not a hard requirement), I won't be able to register my spans and suppress auto-instrumented ones. It's just an extra limitation.

I also understand that public API is a big pain and if making it internal can get instrumentation-api closer to stability, it's probably more important. And once (if) layering and suppression lands in the cross-language spec, we'll expose it back in some way.

@lmolkova
Copy link
Contributor

And more to

I don't use instrumentation-api to instrument my library

if my understanding is correct, instrumentation-api is a super-useful convenience layer, but it probably has limitations compared to the direct usage of tracing and metrics APIs. If I want to go low lever and use them directly, I'd still need all the knobs to control everything that instrumentation-api can control.
I.e. hiding all the APIs to control and interact with instrumentation-api is not a great long-term approach (but maybe a good short term solution)

@iNikem
Copy link
Contributor

iNikem commented Dec 13, 2021

I want to remind you that spec explicitly discourages direct access to context keys:

It is recommended that concerns mediate data access via an API, rather than provide direct public access to their keys.

Also in context Trace API:

API users SHOULD NOT have access to the Context Key used by the Tracing API implementation.

I think this recommendation is applicable here as well

@lmolkova
Copy link
Contributor

lmolkova commented Dec 13, 2021

@iNikem SpanKey and its instances is not a ContextKey, it's an API that hides one.

@trask
Copy link
Member

trask commented Dec 14, 2021

And once (if) layering and suppression lands in the cross-language spec, we'll expose it back in some way.

👍

@trask trask merged commit 09beb58 into open-telemetry:main Dec 14, 2021
@mateuszrzeszutek mateuszrzeszutek deleted the move-SpanKey-to-internal branch December 15, 2021 10:03
RashmiRam pushed a commit to RashmiRam/opentelemetry-auto-instr-java that referenced this pull request May 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants