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

feat(jaeger): remove internal message queue between exporter and exporting tasks #848

Merged
merged 4 commits into from Jul 25, 2022

Conversation

TommyCpp
Copy link
Contributor

#781 introduced an internal queue between jaeger exporter's export method and uploader that actually handles the exporting because Uploader's upload methods takes a mutable reference and hence cannot be called concurrently.

The internal queue helped unblock #781 but has following problems:

  • when sending to collectors we use HTTP clients and those clients are Send + Sync thus the queue is not needed.
  • The queue has an arbitrary capacity of 64, if we can concurrently export more than 64 spans it can result in span dropping.

This PR proposes that:

  • Make upload takes immutable self so that we can call it concurrently.
  • Internally add lock around agent uploader
  • Remove the internal queue inside jaeger exporter.

@TommyCpp TommyCpp added the integration tests Run integration tests label Jul 23, 2022
@codecov
Copy link

codecov bot commented Jul 24, 2022

Codecov Report

Merging #848 (337694a) into main (be0e1f6) will increase coverage by 0.0%.
The diff coverage is 34.4%.

@@          Coverage Diff          @@
##            main    #848   +/-   ##
=====================================
  Coverage   68.9%   69.0%           
=====================================
  Files        115     115           
  Lines       9371    9344   -27     
=====================================
- Hits        6462    6452   -10     
+ Misses      2909    2892   -17     
Impacted Files Coverage Δ
opentelemetry-jaeger/src/exporter/mod.rs 54.6% <18.7%> (+3.3%) ⬆️
opentelemetry-jaeger/src/exporter/uploader.rs 18.7% <20.0%> (-0.7%) ⬇️
...emetry-jaeger/src/exporter/config/collector/mod.rs 58.3% <66.6%> (ø)
opentelemetry-jaeger/src/exporter/config/agent.rs 58.0% <75.0%> (ø)
...aeger/src/exporter/config/collector/http_client.rs 23.0% <100.0%> (ø)
opentelemetry-api/src/context.rs 82.9% <0.0%> (-6.0%) ⬇️
opentelemetry-jaeger/src/exporter/agent.rs 13.4% <0.0%> (+0.9%) ⬆️
opentelemetry-sdk/src/metrics/controllers/push.rs 83.3% <0.0%> (+3.3%) ⬆️
opentelemetry-api/src/trace/mod.rs 76.5% <0.0%> (+4.5%) ⬆️

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 be0e1f6...337694a. Read the comment docs.

@TommyCpp TommyCpp marked this pull request as ready for review July 25, 2022 06:09
@TommyCpp TommyCpp requested a review from a team as a code owner July 25, 2022 06:09
Copy link
Member

@jtescher jtescher left a comment

Choose a reason for hiding this comment

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

Looks good

@jtescher jtescher merged commit d65d545 into open-telemetry:main Jul 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration tests Run integration tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants