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

Remove support to override create key #1887

Merged
merged 1 commit into from
Oct 27, 2020

Conversation

bogdandrutu
Copy link
Member

Indeed this removes one of the testing case otelAsGrpc, but we should not expose an API just for this purpose until we know for sure we need it (applying the rule of minimal API).

Also the createKey it is a strange method on the Storage (which should encapsulate only methods related to storage, and the creation of the key is a Context property).
In the future if needed this should be a method on the Context or other ContextKeyFactory, not on the Storage.

Signed-off-by: Bogdan Drutu bogdandrutu@gmail.com

Indeed this removes one of the testing case otelAsGrpc, but we should not expose an API just for this purpose until we know for sure we need it (applying the rule of minimal API).

Also the createKey it is a strange method on the Storage (which should encapsulate only methods related to storage, and the creation of the key is a Context property).
In the future if needed this should be a method on the Context or other ContextKeyFactory, not on the Storage.

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
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.

The otelAsGrpc case is the true interop case and most important. It's definitely going to be important as I start to add context interop in the instrumentation (took a week to update the SDK dependency in instrumentation due to several Java 8 bytecode issues but finally ready!).

The naming is definitely an issue, how about renaming ContextStorage to something more generic, maybe ContextManager, ContextProvider?

@bogdandrutu
Copy link
Member Author

The otelAsGrpc case is the true interop case and most important. It's definitely going to be important as I start to add context interop in the instrumentation (took a week to update the SDK dependency in instrumentation due to several Java 8 bytecode issues but finally ready!).

Not sure, I think the otelInGrpc is more performant (less wrappers), and it is still interop, context will get propagated based on the grpc propagation.

The naming is definitely an issue, how about renaming ContextStorage to something more generic, maybe ContextManager, ContextProvider?

I think we need 2 classes ContextStorage for the storage/propagation part (which uses a Context object), and another class ContextKeyManager for the Key part. Then you can put them into one ContextProvider but I would not mix these 2 different functionalities.

@anuraaga
Copy link
Contributor

context will get propagated based on the grpc propagation.

But I think if instrumentation propagates the context, which it often does, changes will get out of sync

https://github.com/open-telemetry/opentelemetry-java/blob/master/context/src/otelInGrpcTest/java/io/opentelemetry/context/OtelInGrpcTest.java#L114

We had a user report about it

open-telemetry/opentelemetry-java-instrumentation#1313 (comment)

So I'd like to continue to try with the key customization and removing the method would block that.

Then you can put them into one ContextProvider but I would not mix these 2 different functionalities.

Yeah as long as it's one SPI, that sounds nice.

@bogdandrutu
Copy link
Member Author

bogdandrutu commented Oct 27, 2020

I can send a PR for that bug without the key. Let me send it first to see that it is possible without key

@bogdandrutu
Copy link
Member Author

@anuraaga see #1889

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.

Seems this isn't needed, thanks!

@codecov
Copy link

codecov bot commented Oct 27, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@00aa351). Click here to learn what that means.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1887   +/-   ##
=========================================
  Coverage          ?   85.14%           
  Complexity        ?     1462           
=========================================
  Files             ?      177           
  Lines             ?     5774           
  Branches          ?      592           
=========================================
  Hits              ?     4916           
  Misses            ?      652           
  Partials          ?      206           
Impacted Files Coverage Δ Complexity Δ
.../java/io/opentelemetry/context/ContextStorage.java 100.00% <ø> (ø) 1.00 <0.00> (?)
...main/java/io/opentelemetry/context/ContextKey.java 100.00% <100.00%> (ø) 1.00 <1.00> (?)
...ions/trace/jaeger/sampler/PerOperationSampler.java 75.00% <0.00%> (ø) 4.00% <0.00%> (?%)
...ava/io/opentelemetry/opentracingshim/SpanShim.java 44.11% <0.00%> (ø) 14.00% <0.00%> (?%)
...ry/sdk/metrics/AbstractAsynchronousInstrument.java 100.00% <0.00%> (ø) 4.00% <0.00%> (?%)
...extensions/trace/propagation/JaegerPropagator.java 87.34% <0.00%> (ø) 26.00% <0.00%> (?%)
...ions/trace/jaeger/sampler/JaegerRemoteSampler.java 74.50% <0.00%> (ø) 9.00% <0.00%> (?%)
...in/java/io/opentelemetry/DefaultOpenTelemetry.java 98.36% <0.00%> (ø) 17.00% <0.00%> (?%)
...in/java/io/opentelemetry/metrics/DefaultMeter.java 100.00% <0.00%> (ø) 15.00% <0.00%> (?%)
.../src/main/java/io/opentelemetry/baggage/Entry.java 85.71% <0.00%> (ø) 6.00% <0.00%> (?%)
... and 169 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 00aa351...d03db8f. Read the comment docs.

@bogdandrutu bogdandrutu merged commit 0a3aaeb into open-telemetry:master Oct 27, 2020
@bogdandrutu bogdandrutu deleted the rmcreatekey branch October 27, 2020 17:46
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.

2 participants