[test] Refactor integration tests to directly write/read ptrace.Traces
#7812
+313
−148
Conversation
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
Signed-off-by: Manik Mehta <[email protected]>
Signed-off-by: Manik Mehta <[email protected]>
Contributor
Author
|
Currently fixing the ES tests |
Signed-off-by: Manik Mehta <[email protected]>
Signed-off-by: Manik Mehta <[email protected]>
Signed-off-by: Manik Mehta <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7812 +/- ##
==========================================
- Coverage 95.53% 95.52% -0.02%
==========================================
Files 307 307
Lines 15911 15911
==========================================
- Hits 15201 15199 -2
- Misses 558 559 +1
- Partials 152 153 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Metrics Comparison SummaryTotal changes across all snapshots: 32 Detailed changes per snapshotsummary_metrics_snapshot_cassandra📊 Metrics Diff SummaryTotal Changes: 0
summary_metrics_snapshot_badger📊 Metrics Diff SummaryTotal Changes: 32
❌ Removed Metrics
View diff sampleView diff sampleView diff sampleView diff sampleView diff sampleView diff sampleView diff sampleView diff sampleView diff sampleView diff sampleView diff sampleView diff sampleView diff sampleView diff sampleView diff sampleView diff sampleTotal Changes: 0
|
Signed-off-by: Manik Mehta <[email protected]>
Contributor
Author
|
Majority of tests are passing, except kafka. Working on it!. @yurishkuro Please can you review it! |
Signed-off-by: Manik Mehta <[email protected]>
Contributor
Author
|
Want to ask about kafka, do kafka also assigns one span per resource span? Like ES/OS? Also I can't understand the encoding part, how is kafka tested? i mean to ask how it is different from other storage tests? |
Manik2708
commented
Jan 2, 2026
| return 0 | ||
| } | ||
|
|
||
| func checkSize(t *testing.T, expected *model.Trace, actual *model.Trace) { |
Contributor
Author
There was a problem hiding this comment.
no need to check size, ptracetest.CompareTraces checks for us
Manik2708
commented
Jan 2, 2026
| ptracetest.IgnoreSpansOrder(), | ||
| } | ||
| if err := ptracetest.CompareTraces(expected, actual, options...); err != nil { | ||
| t.Logf("Actual trace and expected traces are not equal: %v", err) |
Contributor
Author
There was a problem hiding this comment.
no need of pretty diff, CompareTraces gives first point of difference as error
Member
There was a problem hiding this comment.
have you tried comparing JSON strigns instead? I think it's nice to get a full dump of diffs, not the first breaking point.
Example:
import (
"github.com/hexops/gotextdiff"
"github.com/hexops/gotextdiff/myers"
"github.com/hexops/gotextdiff/span"
)
func DiffStrings(want, got string) string {
edits := myers.ComputeEdits(span.URIFromPath("want.json"), want, got)
diff := gotextdiff.ToUnified("want.json", "got.json", want, edits)
return fmt.Sprint(diff)
}
Manik2708
commented
Jan 2, 2026
| return bytes.Compare(aAttrs[:], bAttrs[:]) | ||
| } | ||
|
|
||
| func compareTimestamps(a, b pcommon.Timestamp) int { |
Contributor
Author
There was a problem hiding this comment.
we could directly subtract the timestamps but that would require unnecessary and risky conversion of uint64 to int
Manik2708
commented
Jan 2, 2026
| t.Log(err) | ||
| return false | ||
| } | ||
| if len(expected) != len(traces) { |
Contributor
Author
There was a problem hiding this comment.
We don't know the number of traces which reader will give in one slice, so this check becomes useless
Contributor
Author
|
Another problem is that for |
Signed-off-by: Manik Mehta <[email protected]>
yurishkuro
reviewed
Jan 2, 2026
yurishkuro
reviewed
Jan 2, 2026
yurishkuro
reviewed
Jan 2, 2026
| } | ||
| } | ||
| } | ||
| return assert.ObjectsAreEqualValues(expected, actual) |
Member
There was a problem hiding this comment.
I thought ObjectsAreEqualValues does not work for ptrace objects
Contributor
Author
There was a problem hiding this comment.
yes, but they never compare ptrace objects in tests, only operations, services, dependency link etc
yurishkuro
reviewed
Jan 2, 2026
yurishkuro
reviewed
Jan 2, 2026
yurishkuro
reviewed
Jan 2, 2026
yurishkuro
requested changes
Jan 2, 2026
Member
yurishkuro
left a comment
yurishkuro
left a comment
There was a problem hiding this comment.
similar comments in different functions, please address comprehensively
Signed-off-by: Manik Mehta <[email protected]>
Contributor
Author
This issue seemed to be fixed now! I think |
Contributor
Author
|
@yurishkuro The OTLP Json encoding test for large trace is failing with this log (I incremented number of traces from 200 to 8000 in |
Member
Are you generating more verbose traces which exceeded the message limit? There should be settings in both Kafka broker and exporter about max message size. Alternatively we can use a smaller batch in the collector config, there's no reason to send all 1000 spans as a single message |
Contributor
Author
|
Member
|
I remember there was an outstanding ticket in upstream OTEL contrib about Kafka exporter not respecting max batch size / message size, i.e. the batch processor can make the message bigger by aggregating multiple |
Contributor
Author
yes, the issue is still there! |
Signed-off-by: Manik Mehta <[email protected]>
Manik2708
commented
Jan 8, 2026
| _ io.Closer = (*traceWriter)(nil) | ||
|
|
||
| MaxChunkSize = 35 // max chunk size otel kafka export can handle safely. | ||
| MaxChunkSize = 5 // max chunk size otel kafka export can handle safely. |
Contributor
Author
There was a problem hiding this comment.
reducing to this has passed the tests
Contributor
Author
|
@yurishkuro Please review |
yurishkuro
reviewed
Jan 9, 2026
| loadAndParseJSONPB(t, fileName, &trace) | ||
| return &trace | ||
| // getNormalisedTraces normalise traces and assign one resource span to one span | ||
| func getNormalisedTraces(td ptrace.Traces) ptrace.Traces { |
Member
There was a problem hiding this comment.
use usually use American spelling
Suggested change
| func getNormalisedTraces(td ptrace.Traces) ptrace.Traces { | |
| func getNormalizedTraces(td ptrace.Traces) ptrace.Traces { |
I am not clear why this function is needed.
yurishkuro
reviewed
Jan 10, 2026
| require.NoError(t, err, "Not expecting error when unmarshaling fixture %s", path) | ||
| } | ||
|
|
||
| func correctTimeForTraces(trace ptrace.Traces) { |
Member
There was a problem hiding this comment.
and move to dates.go with dates_test.go
| ptracetest.IgnoreSpansOrder(), | ||
| } | ||
| if err := ptracetest.CompareTraces(expected, actual, options...); err != nil { | ||
| t.Logf("Actual trace and expected traces are not equal: %v", err) |
Member
There was a problem hiding this comment.
have you tried comparing JSON strigns instead? I think it's nice to get a full dump of diffs, not the first breaking point.
Example:
import (
"github.com/hexops/gotextdiff"
"github.com/hexops/gotextdiff/myers"
"github.com/hexops/gotextdiff/span"
)
func DiffStrings(want, got string) string {
edits := myers.ComputeEdits(span.URIFromPath("want.json"), want, got)
diff := gotextdiff.ToUnified("want.json", "got.json", want, edits)
return fmt.Sprint(diff)
}
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Which problem is this PR solving?
Description of the changes
ptrace.Tracesdirectly.How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:npm run lintandnpm run test