Use the grpc.ClientConn to handle connections for the otlptracegrpc client #2329
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## main #2329 +/- ##
=======================================
+ Coverage 73.3% 74.3% +0.9%
=======================================
Files 174 173 -1
Lines 12243 12174 -69
=======================================
+ Hits 8986 9049 +63
+ Misses 3019 2888 -131
+ Partials 238 237 -1
|
|
Looks like a good start. |
MadVikingGod
reviewed
Nov 8, 2021
MrAlias
force-pushed
the
poc-grpc-clientconn
branch
3 times, most recently
from
19b8f86
to
30c61db
Compare
MrAlias
changed the title
MrAlias
requested review from
Aneurysm9,
dashpole,
evantorrie,
jmacd,
paivagustavo,
pellared and
XSAM
as code owners
Aneurysm9
approved these changes
Nov 18, 2021
This is not needed now that no tests relies on the listener to wait for a connection to be established before continuing.
Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>
Do not check the second call to the client Stop. There is no guarantee it will not error in normal operation.
MrAlias
force-pushed
the
poc-grpc-clientconn
branch
from
582bf20
to
7067fe0
Compare
|
I'm not 100% sure we can deprecated the options that are deprecated in this PR. We support the configuration of the exporter via environment variables, and they configure the deprecated options. In order to keep this support, I think we will need to persist the options. |
Use the internals of the client to explicit cancel the context returned from exportContext. This gets around the bug where the select in Stop may randomly choose the non-context Done case and avoid returning an error (also failing to cancel the context).
To configure the client/exporter with environment variables these options are used. There is no way to fully remove these options without removing support for configuration with environment variables. Leave that decision and strategy determination to a separate PR.
|
I've removed the deprecation for now. Leaving the decision and strategy determination for a separate Issue/PR. |
pellared
approved these changes
Nov 22, 2021
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Clientuse the underlying gRPCClientConnto handle name resolution, TCP connection establishment (with retries and backoff) and TLS handshakes, and handling errors on established connections by re-resolving the name and reconnecting.Deprecate configuration options for the otlptracegrpcClientthat configure agrpc.ClientConnmanaged by theClientitself. Instead favor the caller passing an already configuredgrpc.ClientConn.This does not include changes to the
otlpmetricgrpcClient. A similar PR is planned as a follow-on to address those changes.Resolve #1527
Resolve #2384