fix: add logging to ES TraceWriter for silent write failures#8237
Open
majiayu000 wants to merge 5 commits intojaegertracing:mainfrom
Open
fix: add logging to ES TraceWriter for silent write failures#8237majiayu000 wants to merge 5 commits intojaegertracing:mainfrom
majiayu000 wants to merge 5 commits intojaegertracing:mainfrom
Conversation
The ES TraceWriter.WriteTraces() had no logging, making it impossible to diagnose cases where archive writes silently produced no spans. Add a warning log when ToDBModel returns zero spans and a debug log for successful writes. Fixes jaegertracing#8058 Signed-off-by: majiayu000 <1835304752@qq.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves diagnosability of Elasticsearch trace archive writes by adding structured logging to the v2 ES TraceWriter when trace conversion produces no spans and when span writes succeed.
Changes:
- Add a
loggerfield toTraceWriter(wired fromSpanWriterParams.Logger). - Emit a warning when
ToDBModelproduces zero DB spans, and a debug log when spans are written. - Update and extend unit tests, including a new empty-traces test case.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/storage/v2/elasticsearch/tracestore/writer.go | Adds logger to TraceWriter and logs on zero-span conversion / successful writes. |
| internal/storage/v2/elasticsearch/tracestore/writer_test.go | Updates tests to construct TraceWriter with a logger and adds an empty-traces test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add observability to the archive trace flow to help diagnose issues like jaegertracing#8058 where writes to Elasticsearch are silently dropped. - TraceWriter.WriteTraces: distinguish empty input (Debug) from non-empty trace data that produced zero spans after conversion (Warn) - QueryService.ArchiveTrace: log request received, completion, and failure with trace_id for end-to-end traceability - Pass logger from telemetry to QueryServiceOptions Signed-off-by: majiayu000 <1835304752@qq.com>
Add nil check for p.Logger in NewTraceWriter, defaulting to
zap.NewNop() to prevent nil-pointer panics when callers pass
telemetry.Settings{} with a nil Logger.
Signed-off-by: majiayu000 <1835304752@qq.com>
- Return early before calling ToDBModel for truly empty traces - Add scope_spans and spans counts to the warning log for better debugging - Use zaptest/observer assertions for all log paths in tests Signed-off-by: majiayu000 <1835304752@qq.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
When p.Logger is nil, NewTraceWriter defaulted to zap.NewNop() for its own logger but still passed the original nil p.Logger to spanstore.NewSpanWriter, which could cause nil-pointer panics. Now p.Logger is also updated before constructing the inner SpanWriter. Signed-off-by: majiayu000 <1835304752@qq.com>
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
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.
Summary
TraceWriterstruct using existingLoggerfromSpanWriterParamsToDBModelreturns 0 spans from non-empty trace data, making silent archive write failures diagnosableWriteTraceswith zeroResourceSpans)Fixes #8058
Test plan
TestTraceWriter_WriteTraces_EmptyTracestest verifies behavior with empty tracesTestTraceWriter_WriteTracesupdated to include loggergo test ./internal/storage/v2/elasticsearch/tracestore/...— all passmake fmt— cleanmake lint— 0 issuesgo test -race ./...) — all pass