Skip to content

Add Context arg to SpanProcessor.OnStart.#1792

Merged
anuraaga merged 1 commit intoopen-telemetry:masterfrom
dynatrace-oss-contrib:add-onstart-context
Oct 14, 2020
Merged

Add Context arg to SpanProcessor.OnStart.#1792
anuraaga merged 1 commit intoopen-telemetry:masterfrom
dynatrace-oss-contrib:add-onstart-context

Conversation

@Oberon00
Copy link
Copy Markdown
Member

Fixes #1785.

return;
}
enqueue(EventType.ON_START, span, null);
enqueue(EventType.ON_START, new AbstractMap.SimpleImmutableEntry<>(span, parentContext), null);
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This seems a bit dirty, so if someone can suggest a better solution that would be good.

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.

I think it's fine for now, although agree it's ugly. I would like to revisit the way the disruptor messages are built at some point, since I ran into difficulties when trying to work with it a few months ago, as well. I think the best answer might be to create a Tuple to hold the message data, or maybe split the message types so they can be more easily customized.

@codecov
Copy link
Copy Markdown

codecov bot commented Oct 13, 2020

Codecov Report

Merging #1792 into master will increase coverage by 0.02%.
The diff coverage is 91.66%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1792      +/-   ##
============================================
+ Coverage     85.94%   85.96%   +0.02%     
  Complexity     1379     1379              
============================================
  Files           166      166              
  Lines          5307     5308       +1     
  Branches        549      549              
============================================
+ Hits           4561     4563       +2     
+ Misses          551      550       -1     
  Partials        195      195              
Impacted Files Coverage Δ Complexity Δ
...ava/io/opentelemetry/sdk/trace/SpanBuilderSdk.java 96.80% <ø> (ø) 31.00 <0.00> (ø)
...telemetry/sdk/trace/export/BatchSpanProcessor.java 89.18% <0.00%> (ø) 8.00 <0.00> (ø)
...elemetry/sdk/trace/export/SimpleSpanProcessor.java 92.30% <ø> (ø) 12.00 <0.00> (ø)
...io/opentelemetry/sdk/trace/MultiSpanProcessor.java 94.44% <100.00%> (ø) 17.00 <0.00> (ø)
.../io/opentelemetry/sdk/trace/NoopSpanProcessor.java 100.00% <100.00%> (ø) 8.00 <1.00> (ø)
...ntelemetry/sdk/trace/RecordEventsReadableSpan.java 81.04% <100.00%> (ø) 67.00 <0.00> (ø)
...ions/trace/export/DisruptorAsyncSpanProcessor.java 85.41% <100.00%> (ø) 10.00 <0.00> (ø)
...k/extensions/trace/export/DisruptorEventQueue.java 81.52% <100.00%> (+0.20%) 13.00 <0.00> (ø)
...k/extensions/trace/jaeger/sampler/RateLimiter.java 94.11% <0.00%> (-5.89%) 4.00% <0.00%> (-1.00%)
...elemetry/opentracingshim/SpanContextShimTable.java 96.42% <0.00%> (+7.14%) 8.00% <0.00%> (+1.00%)

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 939d1b8...670f567. Read the comment docs.

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.

Thanks!

Copy link
Copy Markdown
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.

Thanks!

@anuraaga anuraaga merged commit e539890 into open-telemetry:master Oct 14, 2020
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.

Update SpanProcessor.OnStart to receive parent Context.

3 participants