fix(otel): exporter shutdown data loss + thread safety in span processor#165
Open
pmady wants to merge 3 commits intofuture-agi:mainfrom
Open
fix(otel): exporter shutdown data loss + thread safety in span processor#165pmady wants to merge 3 commits intofuture-agi:mainfrom
pmady wants to merge 3 commits intofuture-agi:mainfrom
Conversation
Both GRPCSpanExporter and HTTPSpanExporter close the session but skip the parent shutdown, which flushes buffered spans. This drops pending trace data on process exit. Fixes part of future-agi#164 Signed-off-by: pmady <pavan4devops@gmail.com>
The snapshot (line 445) and clear (line 458) in SimpleSpanProcessor.shutdown() can race with on_start writes from other threads. Lock only around those two operations per review feedback -- the hot path stays lock-free since dict insert/pop are atomic under GIL. Fixes part of future-agi#164 Signed-off-by: pmady <pavan4devops@gmail.com>
The module-level logger is already defined at line 55 but the shutdown methods use print(). Switch to logger.warning and logger.error so platform teams can route these through their logging pipeline. Partially addresses future-agi#164 finding 5 Signed-off-by: pmady <pavan4devops@gmail.com>
Author
|
this addresses findings 2, 4, and part of 5 from #164. kept the lock scoped to just the shutdown path like you suggested -- lmk if you'd rather see it wider |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
What does this PR do?
Fixes the exporter shutdown bug and thread safety issue from #164.
GRPCSpanExporter.shutdown()andHTTPSpanExporter.shutdown()now callsuper().shutdown()after closing the session, so the parent flushes buffered spansSimpleSpanProcessor.shutdown()takes a lock around the snapshot + clear of_active_spans(per @JayaSurya-27's review -- hot path stays lock-free, dict ops are atomic under GIL)print()tologger.warning/logger.errorin the shutdown methods since the module logger already existsWhy?
The missing
super().shutdown()drops pending spans on process exit. Hits any k8s pod doing graceful shutdown.Fixes #164 (findings 2, 4, partially 5)
How was it tested?
pytest tests/test_config.py tests/test_init.py tests/test_tracers.py-- 67 passed, no regressionsinspect.getsource()that both exporters callsuper().shutdown()test_otel.pyhas a pre-existing import error (SESSION_NAMEremoved) unrelated to this change.Checklist
mainNotes for reviewers
three commits, easy to review individually. lock placement follows @JayaSurya-27's suggestion from the issue thread.
cc @JayaSurya-27