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

autoexport: Add support for metrics #4229

Merged
merged 28 commits into from Oct 24, 2023

Conversation

punya
Copy link
Member

@punya punya commented Aug 22, 2023

Fixes #4131.

Reorganizes autoexport into

  • generic registry and env lookup independent of signal type
  • metric- and trace-specific wrappers around the generic part, to preserve a simple API
  • pre-defined metric and trace factories

Also reorganizes tests to line up with these layers.

@codecov
Copy link

codecov bot commented Aug 22, 2023

Codecov Report

Merging #4229 (503e4ff) into main (72a4bcb) will decrease coverage by 0.1%.
Report is 1 commits behind head on main.
The diff coverage is 86.9%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #4229     +/-   ##
=======================================
- Coverage   80.8%   80.8%   -0.1%     
=======================================
  Files        148     150      +2     
  Lines      10238   10245      +7     
=======================================
+ Hits        8281    8284      +3     
- Misses      1819    1823      +4     
  Partials     138     138             
Files Coverage Δ
exporters/autoexport/registry.go 100.0% <100.0%> (ø)
exporters/autoexport/signal.go 100.0% <100.0%> (ø)
exporters/autoexport/noop.go 66.6% <75.0%> (+23.8%) ⬆️
exporters/autoexport/spans.go 90.9% <90.9%> (ø)
exporters/autoexport/metrics.go 73.3% <73.3%> (ø)

@punya punya marked this pull request as ready for review October 18, 2023 18:30
@punya punya requested review from pellared and a team as code owners October 18, 2023 18:30
Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

I just made a very quick look. I would need to look deeper in the following days. Looks nice so far👍

exporters/autoexport/exporter.go Outdated Show resolved Hide resolved
exporters/autoexport/exporter.go Outdated Show resolved Hide resolved
Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

Looks very good 👍 I have found only one corner case issue.

exporters/autoexport/noop.go Outdated Show resolved Hide resolved
@punya
Copy link
Member Author

punya commented Oct 20, 2023

@pellared @MrAlias this is ready for another round of review.

exporters/autoexport/metrics.go Outdated Show resolved Hide resolved
@dashpole
Copy link
Contributor

From https://github.com/open-telemetry/opentelemetry-specification/blob/563958cb2bd8529990f19fdce7a5f3643bf63091/specification/configuration/sdk-environment-variables.md?plain=1#L222:

The SDK MAY accept a comma-separated list to enable setting multiple exporters.

This would be useful, but doesn't really fit with how the package is currently designed.

Comment on lines +26 to +27
// SpanOption applies an autoexport configuration option.
type SpanOption = option[trace.SpanExporter]
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be TraceOption?

I know we have the SpanExporter, but I wonder if this needs to match that or the corollary to metric: trace.

Copy link
Member

Choose a reason for hiding this comment

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

We can address it in a separate PR if needed.

Personally, I was thinking about renaming to SpanExporterOption.

Let's track it in a separate issue.

Copy link
Member

Choose a reason for hiding this comment

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

@pellared
Copy link
Member

From https://github.com/open-telemetry/opentelemetry-specification/blob/563958cb2bd8529990f19fdce7a5f3643bf63091/specification/configuration/sdk-environment-variables.md?plain=1#L222:

The SDK MAY accept a comma-separated list to enable setting multiple exporters.

This would be useful, but doesn't really fit with how the package is currently designed.

I created #4471

CHANGELOG.md Outdated Show resolved Hide resolved
@pellared pellared merged commit 5adc271 into open-telemetry:main Oct 24, 2023
22 checks passed
@punya punya deleted the autoexport-metrics branch October 24, 2023 16:05
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: Add metrics support
4 participants