Add decoded span assert rules - #12088
Conversation
|
Bits Code status: ✅ Done Comment @DataDog to request changes |
![datadog-datadog-prod-us1-2[bot]](https://cdn.github.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
Please try again by commenting @autotest review.
![chatgpt-codex-connector[bot]](https://cdn.github.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 513469965b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This comment has been minimized.
This comment has been minimized.
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
5134699 to
d7f5363
Compare
|
@autotest review |
There was a problem hiding this comment.
Unordered trace matching can reject a valid trace when a reusable childOfPrevious matcher first probes a non-matching candidate: assertSpan mutates the matcher to that candidate's parent ID, so later candidates are checked against stale state. Keep the resolved parent matcher local instead of mutating the configured matcher.
🤖 Datadog Autotest · Commit d7f5363 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
d7f5363 to
798798a
Compare
There was a problem hiding this comment.
More details
The decoder and assertion paths do not show a confirmed behavioral regression or diff-only production hazard. Adversarial cases were reviewed for malformed JSON, unsigned IDs, missing fields, duplicate/unordered trace matching, ancestry ordering, partial trace chunks, and nested meta_struct values; execution was blocked before tests ran because the checkout requires JDK 25 and the sandbox has only JDK 8/11/17/21.
🤖 Datadog Autotest · Commit 798798a · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 798798ab44
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
798798a to
1004c0a
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1004c0a40d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @Json(name = "meta_struct") | ||
| Map<String, Object> metaStruct; |
There was a problem hiding this comment.
When a meta_struct entry contains numeric leaves, Moshi's generic Object adapter materializes them as Double, while the existing msgpack decoder returns Long for integers and Float for floating-point values in SpanV04.convertValueToObject (SpanV04.java:204-207). Consequently, an exact metaStruct(..., is(expected)) matcher or a predicate casting a numeric leaf can pass with the in-process backend and fail with the dd-apm-test-agent JSON backend; decode these nested numbers into the same representation before exposing the shared DecodedSpan model.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
More details
The decoded-span assertion layer matches the intended behavior for positional, sorted, ancestry-based, unordered, parent-link, default-constraint, and meta_struct checks. No diff-only regression was confirmed; focused Gradle execution was attempted but the sandbox lacks JDK 25 and cannot download it, so validation is limited to source analysis and existing test coverage.
🤖 Datadog Autotest · Commit 1004c0a · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
1004c0a to
8583373
Compare
fa90299 to
8ff8af2
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85833735e9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
8ff8af2 to
3e09e7c
Compare
8583373 to
d100b5a
Compare
Bits found no code fix to apply🟢 Investigated · ⚪ No code fix needed
View in Datadog | Reviewed commit d100b5a · Any feedback? Reach out in #deveng-pr-agent |
dougqh
left a comment
dougqh
left a comment
There was a problem hiding this comment.
Nice - I've wanted something like for a long long time
3e09e7c to
6cba0d6
Compare
d100b5a to
181f643
Compare
6cba0d6 to
98b712f
Compare
d47055e to
62e8fce
Compare
98b712f to
7a8b69b
Compare
There was a problem hiding this comment.
The new unordered/additional-trace matching path aborts instead of skipping an unrelated candidate when childOfPrevious() is evaluated on that candidate's first span. A realistic extra trace with the same span count can therefore fail the smoke test before the valid expected trace is examined; the matcher should report this candidate as a normal mismatch.
🤖 Datadog Autotest · Commit 62e8fce · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| return is(trace.get(this.parentSpanIndex).getSpanId()); | ||
| } else if (this.parentIdMatcher == CHILD_OF_PREVIOUS_MATCHER) { | ||
| if (spanIndex == 0) { | ||
| throw new IllegalStateException("Cannot use childOfPrevious() matcher on the first span"); |
There was a problem hiding this comment.
Smoke tests that intentionally ignore additional traces can fail or abort whenever an unrelated trace has the same span count and is examined before the expected trace.
Assertion details
- Input: Call assertTraces with options -> options.unorder().ignoreAdditionalTraces(), where the first unrelated trace has at least two spans and the matcher includes childOfPrevious() at matcher index 0; place a valid matching trace after it.
- Expected:
The unrelated candidate should be treated as a non-match and the search should continue to the later valid trace. - Actual:
SpanMatcher.parentIdMatcher throws IllegalStateException when childOfPrevious() is checked at span index 0. SmokeTraceAssertions only catches AssertionError while trying candidate traces, so the IllegalStateException escapes and the later valid trace is never examined.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| throw new IllegalStateException("Cannot use childOfPrevious() matcher on the first span"); | |
| throw new AssertionError("Cannot use childOfPrevious() matcher on the first span"); |
There was a problem hiding this comment.
This is on purpose. childOfPrevious() can never be used on first span matcher.
62e8fce to
fd5c521
Compare
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
This PR is rejected because it was updated |
fd5c521 to
b2b41a7
Compare
|
/merge |
|
View all feedbacks in Devflow UI.
This pull request is not mergeable according to GitHub. Common reasons include pending required checks, missing approvals, or merge conflicts — but it could also be blocked by other repository rules or settings.
The expected merge time in
Build pipeline has failing jobs for 0deb35d: What to do next?
DetailsSince those jobs are not marked as being allowed to fail, the pipeline will most likely fail. |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
b22cb26
into
master
2 participants
What Does This Do
This PR introduces decoded span assert rules.
EDIT: This PR unit tests need test agent JSON span deserialization first. I’ll introduce an intermediate PR to the stack to add it. It is still good to review in the meantime.
Motivation
The goal is to provide capabilities for smoke tests to evaluate trace and span structures and values.
Additional Notes
This is mainly inspired from my prior work with instrumentation tests but decoded spans are not like DDSpan. They don’t hold the same values / types so new rules needed to be put in place for smoke tests.
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: APMLP-1247