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

Introduce a ReadWriteSpan interface and pass it to the onStart of the SpanProcessor #1574

Merged
merged 3 commits into from
Aug 24, 2020

Conversation

jkwatson
Copy link
Contributor

Resolves #1569

@codecov
Copy link

codecov bot commented Aug 21, 2020

Codecov Report

Merging #1574 into master will decrease coverage by 0.11%.
The diff coverage is 83.33%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1574      +/-   ##
============================================
- Coverage     86.85%   86.73%   -0.12%     
  Complexity     1392     1392              
============================================
  Files           162      162              
  Lines          5369     5369              
  Branches        513      513              
============================================
- Hits           4663     4657       -6     
- Misses          523      527       +4     
- Partials        183      185       +2     
Impacted Files Coverage Δ Complexity Δ
...io/opentelemetry/sdk/trace/MultiSpanProcessor.java 100.00% <ø> (ø) 17.00 <0.00> (ø)
...ntelemetry/sdk/trace/RecordEventsReadableSpan.java 81.74% <ø> (ø) 76.00 <0.00> (ø)
...telemetry/sdk/trace/export/BatchSpanProcessor.java 94.63% <0.00%> (-1.35%) 8.00 <0.00> (ø)
...elemetry/sdk/trace/export/SimpleSpanProcessor.java 97.36% <ø> (ø) 10.00 <0.00> (ø)
...ions/trace/export/DisruptorAsyncSpanProcessor.java 86.00% <ø> (ø) 10.00 <0.00> (ø)
...k/extensions/trace/export/DisruptorEventQueue.java 83.51% <90.00%> (ø) 12.00 <0.00> (ø)
.../io/opentelemetry/sdk/trace/NoopSpanProcessor.java 100.00% <100.00%> (ø) 8.00 <1.00> (ø)
...try/sdk/metrics/aggregator/LongMinMaxSumCount.java 96.07% <0.00%> (-3.93%) 6.00% <0.00%> (ø%)
...y/sdk/metrics/aggregator/DoubleMinMaxSumCount.java 96.07% <0.00%> (-3.93%) 6.00% <0.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 ca85d8f...f23a997. Read the comment docs.

@Oberon00
Copy link
Member

Note that @bogdandrutu was against having such an interface because it might make people cast the ReadableSpan in OnEnd to it. See open-telemetry/opentelemetry-specification#669 (comment)

@jkwatson
Copy link
Contributor Author

Note that @bogdandrutu was against having such an interface because it might make people cast the ReadableSpan in OnEnd to it. See open-telemetry/opentelemetry-specification#669 (comment)

If they know the SDK codebase, they can cast directly to the RERS implementation as it is today. I don't see that this really changes much, aside from actually matching the current spec. :)

@Oberon00
Copy link
Member

Both PRs match the spec, I intentionally wrote it so that two parameters are also OK. That said, personally I also think that a single ReadWriteSpan interface is more clear.

@jkwatson
Copy link
Contributor Author

Both PRs match the spec, I intentionally wrote it so that two parameters are also OK. That said, personally I also think that a single ReadWriteSpan interface is more clear.

Not sure what other PR you are referencing. Did I miss a PR somewhere?

@Oberon00
Copy link
Member

Oberon00 commented Aug 22, 2020

I mean both the solution on this PR and the solution with two parameters proposed in the description of #1569 (that I also plugged in my comment). I assume that's what you referred to when you said "actually matching the current spec".

@jkwatson
Copy link
Contributor Author

I mean both the solution on this PR and the solution with two parameters proposed in the description of #1569 (that I also plugged in my comment). I assume that's what you referred to when you said "actually matching the current spec".

Oh sorry. I was comparing with the current main branch, not vs another option! Very sorry for the confusion. I do think that either option would meet the spec..this just seemed like the simplest way to go.

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.

Nice!

@jkwatson jkwatson merged commit 7e87a69 into open-telemetry:master Aug 24, 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.

Allow SpanProcessor#onStart to modify received Span
4 participants