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

Use gRPC ClientConn for otlpmetricgrpc client connection handling #2425

Merged
merged 7 commits into from Dec 3, 2021

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Dec 1, 2021

Follow up to #2329.

This updates the otlpmetric module:

  • Add common and reusable retry internal package.
  • Update the otlpmetricgrpc to use the gRPC ClientConn similar to otlptracegrpc.
  • Use the new retry package in the otlpmetrichttp client.

@codecov
Copy link

codecov bot commented Dec 2, 2021

Codecov Report

Merging #2425 (d9f431c) into main (a1f9c97) will increase coverage by 1.3%.
The diff coverage is 74.1%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #2425     +/-   ##
=======================================
+ Coverage   74.2%   75.5%   +1.3%     
=======================================
  Files        175     175             
  Lines      12194   12170     -24     
=======================================
+ Hits        9053    9199    +146     
+ Misses      2904    2722    -182     
- Partials     237     249     +12     
Impacted Files Coverage Δ
.../otlp/otlpmetric/internal/otlpmetrictest/client.go 0.0% <0.0%> (ø)
...ers/otlp/otlpmetric/internal/otlpconfig/options.go 52.9% <3.2%> (-13.0%) ⬇️
...xporters/otlp/otlpmetric/otlpmetrichttp/options.go 69.2% <57.4%> (-30.8%) ⬇️
exporters/otlp/otlpmetric/otlpmetrichttp/client.go 78.7% <71.2%> (-14.7%) ⬇️
exporters/otlp/otlpmetric/otlpmetricgrpc/client.go 97.7% <97.5%> (+3.9%) ⬆️
exporters/otlp/otlpmetric/internal/retry/retry.go 98.2% <98.2%> (ø)
...xporters/otlp/otlpmetric/otlpmetricgrpc/options.go 78.2% <100.0%> (+3.2%) ⬆️
exporters/jaeger/jaeger.go 94.3% <0.0%> (+0.8%) ⬆️

Copy link
Member

@XSAM XSAM left a comment

Choose a reason for hiding this comment

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

Looks good overall.

I might miss some contexts here, the implementation of go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc/client.go looks similar to exporters/otlp/otlptrace/otlptracegrpc/client.go. Any plan to merge these clients? so we do not change twice in the feature.

exporters/otlp/otlpmetric/otlpmetricgrpc/client.go Outdated Show resolved Hide resolved
@MrAlias
Copy link
Contributor Author

MrAlias commented Dec 3, 2021

I might miss some contexts here, the implementation of go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc/client.go looks similar to exporters/otlp/otlptrace/otlptracegrpc/client.go. Any plan to merge these clients? so we do not change twice in the feature.

No, you didn't miss something and you are not alone (#2015). Ever since we refactored the OTLP exporters to be split by signal this duplication has existed. I think the next step now that this will bring the two signals in line is to unify and obscure common code in internal packages/modules. We will likely need to obscure things in internal modules so the whole project does not depend on gRPC or the backoff packages.

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.

None yet

3 participants