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 force-pushed the autoexport-metrics branch from f127c39 to 155c74e Compare October 16, 2023 15:10
@punya punya force-pushed the autoexport-metrics branch from 6f9d996 to 2999fd4 Compare October 16, 2023 15:16
@punya punya marked this pull request as ready for review October 18, 2023 18:30
@punya punya requested a review from pellared as a code owner October 18, 2023 18:30
@punya punya requested a review from a team 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
21 checks passed
@punya punya deleted the autoexport-metrics branch October 24, 2023 16:05
@pellared pellared added this to the untracked milestone Nov 8, 2024
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