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

Log a warning when the BatchSpanProcessor queue is full. #1871

Merged
merged 15 commits into from
Dec 23, 2022

Conversation

johanpel
Copy link
Contributor

@johanpel johanpel commented Dec 15, 2022

Fixes #1870

Changes

Adds a warning when a span is dropped in case the BatchSpanProcessor queue is full.

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • [n/a] Changes in public API reviewed

@codecov
Copy link

codecov bot commented Dec 15, 2022

Codecov Report

Merging #1871 (b2961f9) into main (8866c10) will decrease coverage by 0.06%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1871      +/-   ##
==========================================
- Coverage   85.77%   85.71%   -0.05%     
==========================================
  Files         171      171              
  Lines        5240     5241       +1     
==========================================
- Hits         4494     4492       -2     
- Misses        746      749       +3     
Impacted Files Coverage Δ
sdk/src/trace/batch_span_processor.cc 90.70% <100.00%> (+0.08%) ⬆️
ext/src/http/client/curl/http_client_curl.cc 80.31% <0.00%> (-1.13%) ⬇️

@johanpel johanpel marked this pull request as ready for review December 16, 2022 08:59
@johanpel johanpel requested a review from a team December 16, 2022 08:59
@marcalff
Copy link
Member

Thanks @johanpel for the PR.

FYI, there is a delay in running CI, because we have to manually allow it after a new commit, due to github rules for first time contributors.

I see the CLA is signed already, which is great.

Once this PR is merged, the process will be much easier ;-)

@johanpel
Copy link
Contributor Author

Thanks @johanpel for the PR.

FYI, there is a delay in running CI, because we have to manually allow it after a new commit, due to github rules for first time contributors.

I see the CLA is signed already, which is great.

Once this PR is merged, the process will be much easier ;-)

Makes sense. I've added a few commits to hopefully fix the CI issues. Most were related to the timing around the BatchSpanProcessor. With Valgrind/ASAN/TSAN the BatchSpanProcessor still seems to be able to keep the queue non-full in the BatchSpanProcessorTestPeer/TestManySpansLoss test.

I've reduced the queue size. Hope that gives more consistent results.

@johanpel
Copy link
Contributor Author

It was still a bit too flaky, added a commit to only check logs if anything is actually dropped.

@johanpel johanpel requested a review from owent December 22, 2022 11:02
Copy link
Member

@owent owent left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the fix.

@lalitb lalitb merged commit c0deb40 into open-telemetry:main Dec 23, 2022
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.

Log a warning when BatchSpanProcessor queue is full and span is dropped
4 participants