KEMBAR78
Avoid linear queue.size() calls in span producers by storing queue size separately by raipc · Pull Request #7141 · open-telemetry/opentelemetry-java · GitHub
Skip to content

Conversation

@raipc
Copy link
Contributor

@raipc raipc commented Feb 24, 2025

Related to #7140

@raipc raipc requested a review from a team as a code owner February 24, 2025 11:57
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 24, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

@raipc raipc force-pushed the batch_span_processor__constant_queue_size branch from 779d2c8 to f62710d Compare February 24, 2025 12:47
@codecov
Copy link

codecov bot commented Feb 24, 2025

Codecov Report

Attention: Patch coverage is 71.42857% with 4 lines in your changes missing coverage. Please review.

Project coverage is 89.95%. Comparing base (56941a5) to head (bd2362d).
Report is 55 commits behind head on main.

Files with missing lines Patch % Lines
...a/io/opentelemetry/sdk/trace/internal/JcTools.java 20.00% 4 Missing ⚠️

❌ Your patch status has failed because the patch coverage (71.42%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7141      +/-   ##
============================================
+ Coverage     89.85%   89.95%   +0.10%     
- Complexity     6610     6678      +68     
============================================
  Files           740      750      +10     
  Lines         19986    20168     +182     
  Branches       1964     1977      +13     
============================================
+ Hits          17958    18142     +184     
+ Misses         1439     1434       -5     
- Partials        589      592       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@raipc
Copy link
Contributor Author

raipc commented Feb 24, 2025

JcTools tests exist but jacoco reports are not generated

@jkwatson
Copy link
Contributor

hmm. This modifies a shaded-in dependency. Is it possible to have this fixed upstream, and then get it incorporated in here? I'd hate to have our version diverge from the jctool code, if at all possible.

@raipc
Copy link
Contributor Author

raipc commented Feb 28, 2025

hmm. This modifies a shaded-in dependency. Is it possible to have this fixed upstream, and then get it incorporated in here? I'd hate to have our version diverge from the jctool code, if at all possible.

Could you clarify it, please?

io.opentelemetry.sdk.trace.internal.JcTools is not a shaded class from JcTools library, it's a helper class to work with JcTools queue.
The JCTools library itself is shaded into io.opentelemetry.internal.shaded.jctools package

@jkwatson
Copy link
Contributor

jkwatson commented Mar 1, 2025

hmm. This modifies a shaded-in dependency. Is it possible to have this fixed upstream, and then get it incorporated in here? I'd hate to have our version diverge from the jctool code, if at all possible.

Could you clarify it, please?

io.opentelemetry.sdk.trace.internal.JcTools is not a shaded class from JcTools library, it's a helper class to work with JcTools queue. The JCTools library itself is shaded into io.opentelemetry.internal.shaded.jctools package

whoops. I was misreading. Never mind! Carry on.

…hSpanProcessor.java

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
Copy link
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

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

This looks good to me. Nice optimization!

@jack-berg jack-berg merged commit e6f90f5 into open-telemetry:main Mar 26, 2025
26 of 28 checks passed
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.

4 participants