Skip to content

Allow registering ContextStorage wrappers with code to avoid requirin…#2044

Merged
jkwatson merged 2 commits intoopen-telemetry:masterfrom
anuraaga:contextstorage-wrapper
Nov 12, 2020
Merged

Allow registering ContextStorage wrappers with code to avoid requirin…#2044
jkwatson merged 2 commits intoopen-telemetry:masterfrom
anuraaga:contextstorage-wrapper

Conversation

@anuraaga
Copy link
Copy Markdown
Contributor

@anuraaga anuraaga commented Nov 9, 2020

…g SPI for listening to events.

@sfriberg reached out in wanting to be able to listen ContextStorage events without overriding the detected storage mechanism, and I think being able to register listeners with code is important in any case. It was the first feature request we got in Armeria after adding the ContextStorage SPI since SPI is too tedious to just do something like pushing MDC.

Fixes #922

@codecov
Copy link
Copy Markdown

codecov Bot commented Nov 9, 2020

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2044   +/-   ##
=========================================
  Coverage          ?   85.17%           
  Complexity        ?     1982           
=========================================
  Files             ?      228           
  Lines             ?     7827           
  Branches          ?      831           
=========================================
  Hits              ?     6667           
  Misses            ?      848           
  Partials          ?      312           
Impacted Files Coverage Δ Complexity Δ
.../java/io/opentelemetry/context/ContextStorage.java 50.00% <0.00%> (ø) 2.00 <0.00> (?)
...ain/java/io/opentelemetry/context/LazyStorage.java 77.77% <50.00%> (ø) 9.00 <0.00> (?)
.../opentelemetry/context/ContextStorageWrappers.java 56.25% <56.25%> (ø) 3.00 <3.00> (?)
...nsion/trace/aws/resource/AwsResourceConstants.java 100.00% <0.00%> (ø) 1.00% <0.00%> (?%)
...java/io/opentelemetry/sdk/trace/data/SpanData.java 100.00% <0.00%> (ø) 0.00% <0.00%> (?%)
.../io/opentelemetry/sdk/trace/TracerSharedState.java 93.33% <0.00%> (ø) 10.00% <0.00%> (?%)
...ain/java/io/opentelemetry/api/baggage/Baggage.java 87.50% <0.00%> (ø) 7.00% <0.00%> (?%)
.../opentelemetry/sdk/metrics/InstrumentRegistry.java 100.00% <0.00%> (ø) 6.00% <0.00%> (?%)
...ntelemetry/sdk/trace/RecordEventsReadableSpan.java 84.28% <0.00%> (ø) 70.00% <0.00%> (?%)
.../opentelemetry/sdk/logging/LogSinkSdkProvider.java 100.00% <0.00%> (ø) 7.00% <0.00%> (?%)
... and 221 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 3e31fd9...81f241b. Read the comment docs.

private static final List<Function<? super ContextStorage, ? extends ContextStorage>> wrappers =
new ArrayList<>();

static synchronized void addWrapper(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It probably doesn't matter too much, since this class isn't public, but exposing synchronized methods is generally a bad practice, as it could allow accidental deadlocking if someone synchronizes on this class.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah figured it's ok due to the privateness but went with consistency.

Copy link
Copy Markdown
Contributor

@jkwatson jkwatson left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@sfriberg
Copy link
Copy Markdown
Contributor

Thanks for doing this @anuraaga I think this looks like something that would be easy to use.

Would it make sense to have to be able to register a SPI similar to what is used for the contextstorage and thus instead of manually coding things the ContextStorage init code would scan the class/module path for any classes implementing the ContextStorageWrapper and add those?

@jkwatson
Copy link
Copy Markdown
Contributor

Thanks for doing this @anuraaga I think this looks like something that would be easy to use.

Would it make sense to have to be able to register a SPI similar to what is used for the contextstorage and thus instead of manually coding things the ContextStorage init code would scan the class/module path for any classes implementing the ContextStorageWrapper and add those?

@sfriberg can you log an issue for this, if you think it's a good idea yourself? Thanks!

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.

Context change interceptor

3 participants