fix(otel): ensure OTEL sends final span when -o opt is used #6505
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
eysi09
force-pushed
the
fix-otel-shutdown
branch
5 times, most recently
from
52d5d23
to
ccdaf2c
Compare
eysi09
requested review from
twelvemo
and removed request for
stefreak and
thsig
eysi09
changed the title
vvagaytsev
reviewed
Oct 7, 2024
|
Great! 💯 |
Before this fix, when the -o/--output option was used, Garden would call the `shutdown` function early which prevented the final OTEL span to be emitted. Now we only run the shutdown logic and (wait for output flush) at the very end of the command run. This also means that the `Cli.run()` method no longer has hard-to-predict side effects, depending on what flags were passed.
This option didn't appear to have any real meaning after we moved the call to the `shutdown` function to the parent caller (i.e. the `runCli()` function) as opposed to the `Cli.run()` method so we're removing it here.
Before this fix, attempting to shutdown the OTEL SDK would hang if called before a target exporter is set. The target exporter is set in the Garden class so this would e.g. happen when running 'garden help' and cause a delay before the attempt to shutdown OTEL times out. With this commit we now check whether an exporter is configured, either via env vars or the ReconfigurableExporter, and only attempt to shut down if that's the case. Note that we need to handle this in the enclosing context as opposed to having the ReconfigurableExporter handle it itself because the exporter doesn't "know" if a target exporter will be set later or not.
eysi09
force-pushed
the
fix-otel-shutdown
branch
from
ccdaf2c
to
7091439
Compare
vvagaytsev
approved these changes
Oct 7, 2024
vvagaytsev
approved these changes
Oct 7, 2024
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.
Before this fix, when the -o/--output option was used the last OTEL span wouldn't be sent.
Fixing this surfaced some other issues and the code changes might look odd out of context so I'll try and explain the motivations below (also explained in the individual commit messages):
First commit
First off, the
Cli.run()method would either return the results to the callee which would then do the clean up OR, if a certain combination of flags was passed, it would trigger a process exit itself (via theshutdownfunction) and the cleanup logic in the callee would never run. This was one of the reasons for the OTEL bug.This meant that the behaviour of
Cli.run()was pretty unpredictable and hard to track and the cleanup logic in the callee wasn't guaranteed to run. This was fixed in the first commit and now theCli.run()method always returns the results in a consistent manner and cleanup is done by the callee.Second commit
After this change, it became obvious that the
exitOnAbortflag didn't really do anything so it was removed in the second commit.Third commit
Finally, fixing the OTEL bug with the
Cli.run()changes exposed another bug where callingotelSdk.shutdown()would hang in those cases where an exporter hadn't been configured. This happens when the Garden class isn't initialised AND no OTEL exporter set via theOTEL_*env var. For example when runninggarden help. This was fixed by checking whether an exporter was set in the first place before callingotelSdk.shutdown(). It's a little ugly but the other alternatives I explored didn't feel any better, it's kind of a function of how we use OTEL.What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #6482
Special notes for your reviewer: