Repository navigation
cli-plugins/plugin: add PersistentPostRunE for shutting down otel providers - #7433
jsternberg wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
bcd15ee to
f2a66e4
Compare
| PersistentPostRunE = func(cmd *cobra.Command, _ []string) error { | ||
| persistentPostRunOnce.Do(func() { |
There was a problem hiding this comment.
This will only run for commands that succeed - is this what we want here?
There was a problem hiding this comment.
No, it's not. Why would this not run if the command fails? Do we call os.Exit? I don't see anything in the documentation that mentions the post run doesn't when the command fails.
There was a problem hiding this comment.
Thanks they really should fix the documentation on that to point this out. I guess PersistentPostRunE is just wrong then for this. I'll try to see where else it can go.
| } | ||
| }) | ||
| } | ||
| wg.Wait() |
There was a problem hiding this comment.
Do these Shutdown methods have a guarantee to finish in the given timeout?
If not, perhaps we should run this in a goroutine which we would only wait for the ExportTimeout and if they dont manage to return in that time, just continue with shutdown
There was a problem hiding this comment.
I think so but I'll just add an additional bit of time and do this just in case the Shutdown methods aren't cooperating for some reason.
af5268d to
2927abc
Compare
…viders OTEL providers provided by this library to plugins were not properly shut down. Metrics from the CLI were forcibly flushed in a separate area and the `buildx` command does its own flushing, but this didn't apply to traces. Add shutdown on post run for plugins. This will allow compose to use the tracer provider from this library rather than roll its own custom one. Signed-off-by: Jonathan A. Sternberg <[email protected]>
2927abc to
7f0dd35
Compare
OTEL providers provided by this library to plugins were not properly
shut down. Metrics from the CLI were forcibly flushed in a separate area
and the
buildxcommand does its own flushing, but this didn't apply totraces.
Add shutdown on post run for plugins. This will allow compose to use the
tracer provider from this library rather than roll its own custom one.