Another batch of cleanups in otlp exporter #1357
Merged
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
krnowak
requested review from
Aneurysm9,
evantorrie,
jmacd,
lizthegrey,
MrAlias and
XSAM
as code owners
krnowak
force-pushed
the
otlp-exporter-2
branch
from
2e2ea79
to
d8f9862
Compare
Codecov Report
@@ Coverage Diff @@
## master #1357 +/- ##
======================================
Coverage 77.4% 77.5%
======================================
Files 124 124
Lines 6104 6128 +24
======================================
+ Hits 4730 4754 +24
+ Misses 1125 1123 -2
- Partials 249 251 +2
|
krnowak
force-pushed
the
otlp-exporter-2
branch
from
d8f9862
to
782ce51
Compare
If we will need to maintain more than one connection in future, this splitting off will come in handy. Co-authored-by: Stefan Prisca <stefan.prisca@gmail.com>
There is another channel that serves as a one-time signal, where channel's data type does not matter.
This is to make clear that the lock is guarding only the connection since it can be changed by multiple goroutines, and other members are either atomic or read-only.
The stop channel was rather useless on the exporter side - the primary reason for existence of this channel is to stop a background reconnecting goroutine. Since the goroutine lives entirely within grpcConnection object, move the stop channel here. Also expose a function to unify the stop channel with the context cancellation, so exporter can use it without knowing anything about stop channels. Also make export functions a bit more consistent.
krnowak
force-pushed
the
otlp-exporter-2
branch
from
782ce51
to
5ca7885
Compare
Aneurysm9
approved these changes
Nov 23, 2020
jmacd
approved these changes
Nov 24, 2020
XSAM
approved these changes
Nov 24, 2020
MrAlias
reviewed
Nov 24, 2020
It's possible that both disconnected channel and stop channel will be triggered around the same time, so the goroutine is as likely to start reconnecting as to return from the goroutine. Make sure we return if the stop channel is closed.
Set clients to nil on connection error, so we don't try to send the data over a bad connection, but return a "no client" error immediately.
krnowak
force-pushed
the
otlp-exporter-2
branch
from
5ca7885
to
84319fc
Compare
It's rather risky to call a callback coming from outside within a critical section. Move it out.
Connecting to the collector may also take its time, so it can be useful in some cases to pass a context with a deadline. Currently we just pass a background context, so this commit does not really change any behavior. The follow-up commits will make a use of it, though.
It makes it possible to limit the time spent on connecting to the collector.
Dialling to grpc service ignored the closing of the stop channel, but this can be easily changed.
That way we can make sure that there won't be a window between closing a connection and waiting for the background goroutine to return, where the new connection could be established.
This member is never nil, unless the Exporter is created like
&Exporter{}, which is not a thing we support anyway.
krnowak
force-pushed
the
otlp-exporter-2
branch
from
84319fc
to
2368f43
Compare
|
Updated. |
MrAlias
approved these changes
Nov 24, 2020
AzfaarQureshi
pushed a commit
to open-o11y/opentelemetry-go
that referenced
this pull request
Dec 3, 2020
* Move connection logic into grpcConnection object
If we will need to maintain more than one connection in future, this
splitting off will come in handy.
Co-authored-by: Stefan Prisca <stefan.prisca@gmail.com>
* Make another channel a signal channel
There is another channel that serves as a one-time signal, where
channel's data type does not matter.
* Reorder and document connection members
This is to make clear that the lock is guarding only the connection
since it can be changed by multiple goroutines, and other members are
either atomic or read-only.
* Move stop signal into connection
The stop channel was rather useless on the exporter side - the primary
reason for existence of this channel is to stop a background
reconnecting goroutine. Since the goroutine lives entirely within
grpcConnection object, move the stop channel here. Also expose a
function to unify the stop channel with the context cancellation, so
exporter can use it without knowing anything about stop channels.
Also make export functions a bit more consistent.
* Do not run reconnection routine when being stopped too
It's possible that both disconnected channel and stop channel will be
triggered around the same time, so the goroutine is as likely to start
reconnecting as to return from the goroutine. Make sure we return if
the stop channel is closed.
* Nil clients on connection error
Set clients to nil on connection error, so we don't try to send the
data over a bad connection, but return a "no client" error
immediately.
* Do not call new connection handler within critical section
It's rather risky to call a callback coming from outside within a
critical section. Move it out.
* Add context parameter to connection routines
Connecting to the collector may also take its time, so it can be
useful in some cases to pass a context with a deadline. Currently we
just pass a background context, so this commit does not really change
any behavior. The follow-up commits will make a use of it, though.
* Add context parameter to NewExporter and Start
It makes it possible to limit the time spent on connecting to the
collector.
* Stop connecting on shutdown
Dialling to grpc service ignored the closing of the stop channel, but
this can be easily changed.
* Close connection after background is shut down
That way we can make sure that there won't be a window between closing
a connection and waiting for the background goroutine to return, where
the new connection could be established.
* Remove unnecessary nil check
This member is never nil, unless the Exporter is created like
&Exporter{}, which is not a thing we support anyway.
* Update changelog
Co-authored-by: Stefan Prisca <stefan.prisca@gmail.com>
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.
This PR is best reviewed commit by commit instead of going through all of them lumped together. The PR basically consists of the cleanups and fixes in otlp exporter I have made while working on #1121 and #1122.