fix: flush buffered OTEL logs - #2772
yashrajshuklaaa wants to merge 1 commit into
Conversation
krisztianfekete
left a comment
krisztianfekete
left a comment
There was a problem hiding this comment.
Thanks for the PR! Would you mind also fixing this on the Python side, address the comments and add test coverage for both runtimes and both signals?
| return | ||
| } | ||
|
|
||
| flushCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), flushTimeout()) |
There was a problem hiding this comment.
Can we give each provider its own budget, or flush logs first?
| // session's last message). | ||
| // Uses its own detached timeout because the request context is typically | ||
| // already canceled by the time deferred cleanup runs. The timeout defaults to | ||
| // 3s and is configurable via KAGENT_TRACE_FLUSH_TIMEOUT_MS. |
There was a problem hiding this comment.
We probably want to rename this now that it covers both traces and logs.
|
@krisztianfekete Thanks for the review. I’ve addressed the comments and updated the PR. fix now covers both Go and Python, flushing logs first while keeping a shared timeout budget. also added test coverage for both logs and traces in both runtimes and renamed the timeout setting to KAGENT_TELEMETRY_FLUSH_TIMEOUT_MS |
8dc13f1 to
bbc2055
Compare
|
@mesutoezdil resolved all comments :) PTAL |
|
@krisztianfekete pls review this |
There was a problem hiding this comment.
This is nested under if tracing_enabled, so with OTEL_LOGGING_ENABLED=true and tracing off the new log flush is never called. Can you move out of the tracing branch?
There was a problem hiding this comment.
Fixed. the post-response flush hook now installs when tracing_enabled or metrics_enabled or logging_enabled. Added test_configure_installs_post_response_flush_with_logs_only to cover the logs-only case.
| flushCtx, cancel := context.WithTimeout( | ||
| context.WithoutCancel(ctx), | ||
| flushTimeout(), | ||
| ) |
There was a problem hiding this comment.
Both implementations share this one deadline, while Python splits the budget with remaining_millis, can you please match the two?
There was a problem hiding this comment.
Matched now. each provider in Go gets its own full FlushTimeout, the same way Python already passed the full timeout_millis to each provider's force_flush individually. No more shared/split deadline in either runtime.
|
|
||
| func flushTimeout() time.Duration { | ||
| if value := strings.TrimSpace(os.Getenv("KAGENT_TRACE_FLUSH_TIMEOUT_MS")); value != "" { | ||
| if value := strings.TrimSpace(os.Getenv("KAGENT_TELEMETRY_FLUSH_TIMEOUT_MS")); value != "" { |
There was a problem hiding this comment.
The rename drops any existing KAGENT_TRACE_FLUSH_TIMEOUT_MS override to the 3s default with no warning. Can we keep the old name as a fallback for a release and log a notice for the rename?
There was a problem hiding this comment.
Moot after the rebase. the telemetry refactor removed the configurable timeout env var entirely in favor of a fixed constant so there's no override or fallback to preserve on either name.
Signed-off-by: Yashraj Shukla <shuklayashraj68@gmail.com>
8d34cf6 to
c70733d
Compare
|
@krisztianfekete all review comments have been addressed. PTAL : ) |
4 participants
What
Fix
ForceFlush()so it flushes both the tracer and logger providers.Why
GenAI audit logs are buffered by the OTEL batch log processor. When an Agent Substrate actor checkpoints right after the A2A response closes, those buffered logs can be left behind and never exported.
Changes
LoggerProviderinForceFlush().Validation
go test ./adk/pkg/telemetry ./adk/pkg/a2a/servergo build ./adk/pkg/telemetry ./adk/pkg/a2a/serverAll passed.
Fixes #2759