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

Print OpenTelemetry Configuration #2475

Closed
wants to merge 8 commits into from

Conversation

joe-elliott
Copy link
Member

@joe-elliott joe-elliott commented Sep 13, 2020

Which problem is this PR solving?

Short description of the changes

  • Consolidation of config factory
  • Addition of config.dump-otel flag
  • Moved zipkin host port configuration into the zipkinreceiver CreateDefaultConfig(). This mimics the way the jaegerreceiver works and removes the ZipkinHostPort field from ComponentSettings.

Notes

  • fmt.Println is crude but was chosen b/c it would be unaffected by config like a logger and cleanly dumps the config to stdout.
  • I am purposefully exiting after printing the config to protect secrets. Otherwise it might be easy to leave this parameter in and deploy it to production and accidentally dump secrets.

Todo

  • Tests

@pavolloffay If you have a chance to review this and make sure I'm headed the right direction please do. I intend to add tests and drop draft status if this change is what is desired.

@joe-elliott joe-elliott changed the title Otel config Print OpenTelemetry Configuration Sep 13, 2020
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott joe-elliott marked this pull request as ready for review September 16, 2020 01:29
@joe-elliott joe-elliott requested a review from a team as a code owner September 16, 2020 01:29
@joe-elliott
Copy link
Member Author

@pavolloffay I think this PR is in a good spot. Added a basic test to make sure that config factory returned an error if the dump otel flag was passed. Let me know what you think.

@jpkrohling
Copy link
Contributor

crossdoc-otel job restarted.

@codecov
Copy link

codecov bot commented Sep 16, 2020

Codecov Report

Merging #2475 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2475      +/-   ##
==========================================
+ Coverage   95.51%   95.53%   +0.01%     
==========================================
  Files         208      208              
  Lines       10750    10750              
==========================================
+ Hits        10268    10270       +2     
+ Misses        406      405       -1     
+ Partials       76       75       -1     
Impacted Files Coverage Δ
plugin/storage/badger/spanstore/reader.go 96.79% <0.00%> (+0.64%) ⬆️

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 df2582d...fd89df4. Read the comment docs.

@pavolloffay
Copy link
Member

The upstream collector does not have a default config in terms of enabled components, however each component creates a component config with default settings that are unknown to the user.

So I think this feature might be also useful in the upstream OTel collector. There might be more ways how to approach this either add a flag that prints the full config or print the component config when a component is being started. I have filed an issue in OTEL open-telemetry/opentelemetry-collector#1791

@joe-elliott
Copy link
Member Author

joe-elliott commented Sep 16, 2020

Sounds good, we can leave this for a bit and then close it if otel collector is ok with adding this functionality.

Oh, what did you think about moving the ZipkinHostPort field? I think that is a good improvement regardless.

@joe-elliott
Copy link
Member Author

joe-elliott commented Oct 1, 2020

Going to close this PR in favor of doing this upstream as linked by @pavolloffay. The refactoring in this PR was taken care of in #2495

@joe-elliott joe-elliott closed this Oct 1, 2020
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.

Print final configuration
3 participants