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

Add a test for the RECORD_ONLY span handling in the BatchSpanProcessor #2440

Merged
merged 1 commit into from
Jan 7, 2021

Conversation

jkwatson
Copy link
Contributor

@jkwatson jkwatson commented Jan 6, 2021

And remove the old TODOs from the BSP test.

resolves #1588

And remove the old TODOs from the BSP test.
@codecov
Copy link

codecov bot commented Jan 6, 2021

Codecov Report

Merging #2440 (5762b13) into master (015708b) will increase coverage by 0.07%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2440      +/-   ##
============================================
+ Coverage     88.56%   88.64%   +0.07%     
- Complexity     2638     2640       +2     
============================================
  Files           303      303              
  Lines          9026     9026              
  Branches        928      928              
============================================
+ Hits           7994     8001       +7     
+ Misses          708      700       -8     
- Partials        324      325       +1     
Impacted Files Coverage Δ Complexity Δ
...telemetry/sdk/trace/export/BatchSpanProcessor.java 85.96% <0.00%> (+5.26%) 10.00% <0.00%> (+1.00%)
...entelemetry/sdk/trace/samplers/SamplingResult.java 85.71% <0.00%> (+7.14%) 6.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 015708b...5762b13. Read the comment docs.

*/
createEndedSpan(SPAN_NAME_2);

List<SpanData> exported = waitingSpanExporter.waitForExport();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need to do a similar pattern as the other test of exporting a sampled span after the record only span and checking it's the only exported span. Otherwise, this isn't doing any waiting I believe since nothing to wait for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The exporter is waiting for 1, and I was relying on the timeout to ensure nothing was getting through. Am I misunderstanding how the waiting exporter works?

@jkwatson jkwatson merged commit aa8433d into open-telemetry:master Jan 7, 2021
@jkwatson jkwatson deleted the add_record_only_test branch January 7, 2021 02:23
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.

Deal with the TODOs in the BatchSpanProcessorTest
3 participants