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

Store SpanShim/Baggage in the Context. #2982

Merged
merged 3 commits into from
Aug 3, 2021

Conversation

carlosalberto
Copy link
Contributor

@carlosalberto carlosalberto commented Mar 4, 2021

Store both the SpanShim and Baggage no make the OT Shim Specification compliant:

  • SpanShim is stored in the Context to avoid creating disposable SpanShim objects when returning the active OT Span.
  • Baggage of the Span is stored in the Context as well, if there's any defined for it.

@codecov
Copy link

codecov bot commented Mar 4, 2021

Codecov Report

Merging #2982 (aba265f) into main (d4bbaf4) will increase coverage by 90.86%.
The diff coverage is 94.11%.

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #2982       +/-   ##
===========================================
+ Coverage        0   90.86%   +90.86%     
- Complexity      0     3234     +3234     
===========================================
  Files           0      368      +368     
  Lines           0     9954     +9954     
  Branches        0     1006     +1006     
===========================================
+ Hits            0     9045     +9045     
- Misses          0      597      +597     
- Partials        0      312      +312     
Impacted Files Coverage Δ
...pentelemetry/opentracingshim/ScopeManagerShim.java 81.25% <88.88%> (ø)
...ava/io/opentelemetry/opentracingshim/SpanShim.java 87.50% <100.00%> (ø)
...opentelemetry/sdk/metrics/data/DoubleExemplar.java 66.66% <0.00%> (ø)
...a/io/opentelemetry/api/baggage/BaggageBuilder.java 100.00% <0.00%> (ø)
...metry/sdk/extension/resources/ProcessResource.java 87.50% <0.00%> (ø)
...lemetry/sdk/trace/samplers/ParentBasedSampler.java 100.00% <0.00%> (ø)
...telemetry/extension/noopapi/NoopOpenTelemetry.java 75.00% <0.00%> (ø)
...entelemetry/sdk/extension/zpages/ZPageHandler.java 50.00% <0.00%> (ø)
...elemetry/exporter/otlp/internal/CommonAdapter.java 93.58% <0.00%> (ø)
.../io/opentelemetry/sdk/trace/NoopSpanProcessor.java 100.00% <0.00%> (ø)
... and 360 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4bbaf4...aba265f. Read the comment docs.

@carlosalberto carlosalberto marked this pull request as ready for review July 26, 2021 13:52
@carlosalberto carlosalberto requested a review from a user July 26, 2021 13:52
@carlosalberto
Copy link
Contributor Author

@open-telemetry/java-approvers Please review again, as this is ready to go in ;)

PS - I decided to not try to make the SpanShim object implement both OT/OTel interfaces for now, in order to expedite a full, correct implementation of the OT Shim portion of the spec. Such potential change will be done in a follow up ;)

@carlosalberto carlosalberto changed the title Initial attempt to store the SpanShim right in the Context. Store SpanShim/Baggage in the Context. Jul 26, 2021
Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious what people think about moving OpenTracing / OpenCensus shims into auto instrumentation and not having actual libraries.

@jkwatson
Copy link
Contributor

jkwatson commented Aug 3, 2021

Curious what people think about moving OpenTracing / OpenCensus shims into auto instrumentation and not having actual libraries.

Aren't there non-autoinstrumentation users who might want these as well?

@jkwatson jkwatson merged commit 8492353 into open-telemetry:main Aug 3, 2021
@anuraaga
Copy link
Contributor

anuraaga commented Aug 4, 2021

Aren't there non-autoinstrumentation users who might want these as well?

I dunno - I recall hearing OpenCensus Shim not being a priority anymore, and I wonder if it's because of the ease of auto instrumentation for most users.

@trask
Copy link
Member

trask commented Aug 5, 2021

@anuraaga and I are also a bit concerned about two layers of bridging at once if someone uses the OpenTracing shim (bridge) + the Javaagent (bridge)

@carlosalberto
Copy link
Contributor Author

@anuraaga Also, most OT users used to be manual instrumentation users, and the ones who used the OT SpecialAgent have so far simply moved directly to OTel auto-instrumentation, no need for the OT Shim.

@anuraaga
Copy link
Contributor

anuraaga commented Aug 6, 2021

@carlosalberto I won't try to make any recommendation for OT I just want to make sure all the options are taken into account. In this case, I recommend thinking not only of the manual/auto axis but OT/OTel axis. By making our "OT shim" be a shim introduced by the java agent, we will have a good story of getting users onto OTel, in fact any instrumentation supported by OTel would be using it with the shim for custom frameworks instrumentation (this requires work on the auto instrumentation side of course to suppress overlapping instrumentation if we happen to go with it). Or if a user loves library instrumentation they can make the code change - it's what I'd probably do for my own frameworks and my understanding is this is where OC is going where they've found that library users tend to not mind just switching over, both agents and shims are off limits for this sort of persona.

It doesn't mean it applies for OT but I don't think we've actually done user research to know the shim needs to live outside a java agent.

@carlosalberto
Copy link
Contributor Author

Actually doing some OT research is not a bad idea. As I said, what I've seen is manual instrumentation for OT mostly, but it would be interesting to see the experience of other companies. I will open an issue in this repo for now, so we don't forget about that possibility.

This was referenced Dec 19, 2021
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