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

exporters/autoexport: add support for comma-separated values for OTEL_{METRICS,TRACES,LOGS}_EXPORTER #5830

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

thomasgouveia
Copy link
Contributor

@thomasgouveia thomasgouveia commented Jul 1, 2024

This PR introduces the support of comma-separated values for the following environment variables:

  • OTEL_TRACES_EXPORTER
  • OTEL_LOGS_EXPORTER
  • OTEL_METRICS_EXPORTER

The functions NewLogExporter, NewMetricReader, and NewSpanExporter are now deprecated but will continue to work to ensure backward compatibility. Users must consider moving to NewLogExporters, NewMetricReaders, and NewSpanExporters to initialize one or more exporters. (See example_test.go).

Unit tests were adapted to use the newly created functions. A test has been added for each signal to ensure the respective deprecated function continues working.

Blocked by #5816 (will need to be rebased once merged)

Closes #4471

@thomasgouveia thomasgouveia requested a review from a team July 1, 2024 14:42
Copy link

codecov bot commented Jul 1, 2024

Codecov Report

Attention: Patch coverage is 83.54430% with 13 lines in your changes missing coverage. Please review.

Project coverage is 65.7%. Comparing base (d0309dd) to head (aaf4d78).

Files Patch % Lines
exporters/autoexport/logs.go 57.1% 5 Missing and 1 partial ⚠️
exporters/autoexport/metrics.go 76.9% 2 Missing and 1 partial ⚠️
exporters/autoexport/spans.go 81.8% 2 Missing ⚠️
exporters/autoexport/utils/env/env.go 83.3% 1 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #5830   +/-   ##
=====================================
  Coverage   65.6%   65.7%           
=====================================
  Files        204     206    +2     
  Lines      12974   13027   +53     
=====================================
+ Hits        8522    8565   +43     
- Misses      4198    4205    +7     
- Partials     254     257    +3     
Files Coverage Δ
exporters/autoexport/factory.go 100.0% <100.0%> (ø)
exporters/autoexport/registry.go 100.0% <100.0%> (ø)
exporters/autoexport/signal.go 100.0% <100.0%> (ø)
exporters/autoexport/spans.go 93.5% <81.8%> (+1.2%) ⬆️
exporters/autoexport/utils/env/env.go 83.3% <83.3%> (ø)
exporters/autoexport/metrics.go 78.6% <76.9%> (-0.3%) ⬇️
exporters/autoexport/logs.go 80.6% <57.1%> (-19.4%) ⬇️

Copy link
Member

@dmathieu dmathieu left a comment

Choose a reason for hiding this comment

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

(haven't gone through the PR yet)

CHANGELOG.md Outdated Show resolved Hide resolved
@dmathieu
Copy link
Member

dmathieu commented Jul 1, 2024

This is a rather large PR which, at a first glance, seems not to include only what's described in the PR title. Could it be split into smaller PRs?

…OTEL_*_EXPORTER (open-telemetry#4471)

This commit introduces the support of comma-separated value for OTEL_{METRICS,TRACES,LOGS}_EXPORTER.
New functions can now be used to intialize a list of exporters: NewMetricReaders, NewLogExporters, NewSpanExporters.

Old ones (NewMetricReader, NewLogExporter, NewSpanExporter) are now deprecated but still continue to do they initial work to avoid breaking change.

Signed-off-by: thomasgouveia <[email protected]>
Signed-off-by: thomasgouveia <[email protected]>
Signed-off-by: thomasgouveia <[email protected]>
Signed-off-by: thomasgouveia <[email protected]>
Signed-off-by: thomasgouveia <[email protected]>
Signed-off-by: thomasgouveia <[email protected]>
@thomasgouveia
Copy link
Contributor Author

Hi @dmathieu, I've updated the PR to remove code changes unrelated to the feature. Let me know if I missed something!

@thomasgouveia
Copy link
Contributor Author

Hi @dmathieu, @pellared, any additional feedback for this one?

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.

autoexport: Accept a comma-separated list to enable setting multiple exporters
3 participants