embed: add integration test for distributed tracing #14348
Conversation
To verify distributed tracing feature is correctly setup, this PR adds an integration test for this feature. In the process of writing the test, I discovered a goroutine leak due to the TraceProvider not being closed. This PR fixs this issue as well. Signed-off-by: Yingrong Zhao <[email protected]>
server/embed/config_tracing.go
Outdated
| type tracingExporter struct { | ||
| exporter tracesdk.SpanExporter | ||
| opts []otelgrpc.Option | ||
| shutdown func(ctx context.Context) |
There was a problem hiding this comment.
The name shutdown is a little strange to me, if tracingExporter knows what it should do on close, why not to get the traceProvider added to the struct directly? Two choice:
- Add the
traceProviderinto the struct directly, and callte.traceProvider.Shutdown(ctx)directly on Close. - Rename to
cleanupso that the logic is agnostic totracingExporter, instead it's just doing the cleanup task it's told.
There was a problem hiding this comment.
Thank you for the suggestion. I went with the first option
| if err != nil { | ||
| return e, err | ||
| } | ||
| if tracingExporter == nil || len(opts) == 0 { | ||
| return e, fmt.Errorf("error setting up distributed tracing") |
There was a problem hiding this comment.
You intentionally removed this, does it mean that tracingExporter == nil || len(opts) == 0 will never be true if the returned err is nil?
There was a problem hiding this comment.
Yes, that's the intention. I noticed no code path would result in err == nil and either exporter nor opts would be nil
Signed-off-by: Yingrong Zhao <[email protected]>
| } | ||
| if te.provider != nil { | ||
| te.provider.Shutdown(ctx) | ||
| } |
There was a problem hiding this comment.
I am not quite confident on this. Have you considered the order of the shutdown? In other words, is there any potential issue when shutting down the exporter firstly? I just had a quick code review on the otl SDK, it seems that the provider callbacks the exporters; if we shutdown the exporter firstly, will it cause any potential temporary issue on the provider?
Have you thought about shutting down the provider firstly?
Please clarify this.
There was a problem hiding this comment.
From my understanding, if the exporter is shutdown first, the provider will either hit a timeout or a shutdown error. It would mean that some spans that hasn't been sent to the remote may be lost. Thank you for pointing it out. I changed it to shutdown the provider first.
|
Please run |
|
@VinozzZ There are lots of comment changes in this PR. Did you do it intentionally or they are just updated by IDE or any tool? If it's the former (intentionally did), please do it in a separate PR. If it's the latter, please revert the changes. |
This is the change from running |
Signed-off-by: Yingrong Zhao <[email protected]>
VinozzZ
force-pushed
the
add-integration-test-for-tracing
branch
from
64ae889 to
57206f9
Compare
To verify distributed tracing feature is correctly setup, this PR adds
an integration test for this feature.
In the process of writing the test, I discovered a goroutine leak due to
the TraceProvider not being closed. This PR fixs this issue as well
Closes #13814