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): better configuration pipeline. #748

Merged
merged 7 commits into from
Mar 13, 2022

Conversation

TommyCpp
Copy link
Contributor

@TommyCpp TommyCpp commented Mar 8, 2022

I think the jaeger configuration pipeline has been a little hard to understand between agent's configurations and collector's configurations. This PR:

  • Separate agent pipeline and collector pipeline. it's now new_agent_pipeline and new_collector_pipeline. Note that this means if the collector exporter to start we will not fall back to agent exporter.
  • Add AgentPipeline and CollectorPipeline to configure agent exporter and collector exporter.
  • Add agent only configurations(auto_batches_split, max_packet_size)
  • Add collector only configurations(4 build-in http clients, timeout, username, password and endpoint)
  • Rename with_agent_endpoint to with_endpoint as agent pipeline and collector pipeline are separate now. Similar to collector.
  • Add Configurable trait to include common attributes shared by agent pipeline and collector pipeline.
  • Removed with_tag method.
  • Make build-in HTTP client features(surf_collector_client, isahc_collector_client, reqwest_collector_client, reqwest_collector_blocking_client) additive.
  • Make collector_client and wasm_collector_client additive. Now users can start a wasm collector using wasm_collector_client.
  • Add a ConfigError for all error from the builder. Most of them are invalid value like string that cannot convert to URI
  • Update the docs

Overall we seprate the collector configurations from agent configurations to prevent users set unrelevant configurations. For example,

let tracer = new_pipeline()
                 .with_agent_endpoint(...) // This confiugration will not be used if the collector is built successfully
                 .with_collector_pipeline(...)
                 .install_simple()

The downside is now if the collector fails to build, we no longer fall back to agent.

BREAKING CHANGES:
As we separate the agent pipeline and collector pipeline. The user must understand which one they are using and change new_pipeline to new_collector_pipeline or new_agent_pipeline.

Functions that contains agent or collector will no longer contains it as the pipeline are different. For example, with_collector_username will now be with_username.

For users using build in http client(isahc, reqwest, reqwest-blocking or surf). They need to add a new function to pipeline that explicitly point out the http client they want to use.

#736

- Separate agent pipeline and collector pipeline. it's now `new_agent_pipeline` and `new_collector_pipeline`
- Add `Configurable` trait to include common attributes shared by agent pipeline and collector pipeline.
- Removed `with_tag` method.
- Make build in http client additive. `surf_collector_client`, `isahc_collector_client`, etc. are now just allow user to choose the http client.
@codecov
Copy link

codecov bot commented Mar 8, 2022

Codecov Report

Merging #748 (498b5bc) into main (e09392d) will decrease coverage by 1.17%.
The diff coverage is 50.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #748      +/-   ##
==========================================
- Coverage   70.19%   69.01%   -1.18%     
==========================================
  Files         108      111       +3     
  Lines        8804     8973     +169     
==========================================
+ Hits         6180     6193      +13     
- Misses       2624     2780     +156     
Impacted Files Coverage Δ
opentelemetry-jaeger/src/exporter/agent.rs 12.50% <ø> (+0.50%) ⬆️
opentelemetry-jaeger/src/exporter/runtime.rs 0.00% <ø> (ø)
opentelemetry-jaeger/src/lib.rs 93.04% <ø> (ø)
opentelemetry-jaeger/src/exporter/collector.rs 19.00% <15.38%> (-48.86%) ⬇️
...aeger/src/exporter/config/collector/http_client.rs 25.88% <25.88%> (ø)
opentelemetry-jaeger/src/exporter/uploader.rs 19.35% <33.33%> (-4.65%) ⬇️
opentelemetry-jaeger/src/exporter/config/agent.rs 42.85% <42.85%> (ø)
opentelemetry-jaeger/src/exporter/mod.rs 56.91% <50.00%> (-2.85%) ⬇️
...emetry-jaeger/src/exporter/config/collector/mod.rs 57.96% <57.96%> (ø)
opentelemetry-jaeger/src/exporter/config/mod.rs 83.09% <83.09%> (ø)
... and 13 more

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 e09392d...498b5bc. Read the comment docs.

@TommyCpp TommyCpp marked this pull request as ready for review March 8, 2022 05:04
@TommyCpp TommyCpp requested a review from a team March 8, 2022 05:04
@TommyCpp
Copy link
Contributor Author

TommyCpp commented Mar 8, 2022

I think I have a way to resolve the msrv error. But it will expose HasRequiredConfig trait. Maybe we should just bump the MSRV?

@djc
Copy link
Contributor

djc commented Mar 8, 2022

What would we need to bump it to?

@TommyCpp
Copy link
Contributor Author

TommyCpp commented Mar 9, 2022

Dig a little bit and looks like the error was relaxed by rust-lang/rust#90586.
Since we cannot bring the MSRV to 1.59. I will make HasRequiredConfig public Actually we can seal it

@TommyCpp TommyCpp force-pushed the jaeger-additive-clients branch from 1d1f03d to ac74d94 Compare March 10, 2022 06:41
@TommyCpp TommyCpp requested a review from jtescher March 12, 2022 22:18
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.

3 participants