Refactored Jaeger exporter to move connection into start for upcoming Auth changes. #3299
Conversation
pavankrish123
changed the title
jpkrohling
dismissed
their stale review
Taking this back, see comment about the state reporter
| @@ -264,7 +278,10 @@ func TestConnectionStateChange(t *testing.T) { | |||
| wg.Done() | |||
| }) | |||
|
|
|||
| require.NoError(t, sender.start(context.Background(), componenttest.NewNopHost())) | |||
| go func() { | |||
| sender.startConnectionStatusReporter() | |||
There was a problem hiding this comment.
This looks odd. Shouldn't you be calling start instead?
There was a problem hiding this comment.
If not, you'd need a way to stop the connection reporter. Otherwise, it leaks resources between tests.
There was a problem hiding this comment.
Yeah, we need to call startConnectionStatusReporter explicitly, calling start will create actual grpc connection which we can't control the states. The cleanup in the next line calls the appropriate termination logic in startConnectionStatusReporter. I have added a signal to ensure we wait for the startConnectionStatusReporter comes to end,
|
Please rebase |
|
@jpkrohling please revert this or file a new PR if anything is missing from here. |
Description:
Updated JaegerExporter for upcoming client auth extension changes
Link to tracking Issue:
#3287 #3282
Testing:
Unit tests
Documentation:
Unit tests