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

Do not block Temporality/Aggregation on OTLP metric export #4395

Merged
merged 7 commits into from
Aug 2, 2023

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Jul 31, 2023

Resolve #3925

Closes #4001

Currently for both the HTTP and gRPC OTLP metric exporters when Export is called all calls to Aggregation and Temporality are blocked. This means the whole SDK processing pipeline, which calls these methods, becomes blocked. This is especially bad when Export hangs indefinitely due to an inaccessible target.

Instead of having a universal client lock for a shared Exporter from the otlpmetric/internal package, have each exporter package implement a non-blocking Exporter directly. This removes the dependency on shared internal packages from an external module1.

This does not completely remove the dependency on shared internal packages from an external module. That is left for a separate PR.

Footnotes

  1. https://github.com/open-telemetry/opentelemetry-go/issues/3846

@MrAlias MrAlias added pkg:SDK Related to an SDK package area:metrics Part of OpenTelemetry Metrics labels Jul 31, 2023
@codecov
Copy link

codecov bot commented Jul 31, 2023

Codecov Report

Merging #4395 (37ba558) into main (528a0cb) will increase coverage by 0.1%.
The diff coverage is 88.8%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #4395     +/-   ##
=======================================
+ Coverage   83.4%   83.5%   +0.1%     
=======================================
  Files        184     184             
  Lines      14380   14448     +68     
=======================================
+ Hits       11997   12071     +74     
+ Misses      2155    2147      -8     
- Partials     228     230      +2     
Files Changed Coverage Δ
exporters/otlp/otlpmetric/internal/exporter.go 67.2% <ø> (ø)
...porters/otlp/otlpmetric/otlpmetricgrpc/exporter.go 83.0% <88.6%> (+35.4%) ⬆️
...porters/otlp/otlpmetric/otlpmetrichttp/exporter.go 83.0% <88.6%> (+35.4%) ⬆️
exporters/otlp/otlpmetric/otlpmetricgrpc/client.go 90.6% <100.0%> (+2.8%) ⬆️
exporters/otlp/otlpmetric/otlpmetrichttp/client.go 78.6% <100.0%> (+1.0%) ⬆️

... and 1 file with indirect coverage changes

@MrAlias
Copy link
Contributor Author

MrAlias commented Jul 31, 2023

If merged, supersedes #4001

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.

👍

@MrAlias MrAlias merged commit 378e51e into open-telemetry:main Aug 2, 2023
22 checks passed
@MrAlias MrAlias deleted the otlpmetric-no-block branch August 2, 2023 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:metrics Part of OpenTelemetry Metrics pkg:SDK Related to an SDK package
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Metric creation slowed down by unreachable collector with gRPC
4 participants