Mix Databricks repair attempt into Spark parent trace id - #12022
Conversation
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
There was a problem hiding this comment.
More details
Databricks repair attempt extraction and trace ID mixing implementation is sound. Adversarial testing of the byte parsing logic (12/12 scenarios passed) confirms correct boundary checks, error handling, and output correctness. Constructor signature change is properly propagated with updated test fixtures. Trace ID differentiation between repair attempts (attempt 0 vs 1+) is verified by new test assertion. No behavioral regressions.
📊 Validated against 12 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit 103f0b8 · What is Autotest? · Any feedback? Reach out in #autotest
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 103f0b88f6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // In the java-serialized stream the value follows its key as the next string, written as | ||
| // TC_STRING (0x74) with a 2-byte big-endian length prefix. | ||
| for (int i = keyIdx + JOB_RUN_ATTEMPT_NUM_KEY.length; i + 3 <= decoded.length; i++) { | ||
| if (decoded[i] != 0x74) { |
There was a problem hiding this comment.
Handle repeated serialized string values
When unity.scope.data is a normal Java-serialized map and the attempt value has already appeared earlier in the stream (for example another Databricks tag with value "1" is written before jobRunAttemptNum), serialization emits that value as a TC_REFERENCE (0x71) rather than another TC_STRING (0x74). This loop skips references and then falls back to 0 (or parses a later string), so those repaired runs still hash as attempt 0 and the Spark spans attach to the original Databricks trace instead of the repair trace.
Useful? React with 👍 / 👎.
| def contextWithoutTaskRunId = new DatabricksParentContext(null, "5678", null, 0) | ||
| // A repaired run reuses the same jobRunId but reports a non-zero attempt, which must yield a | ||
| // different trace id (its own trace) while a repaired task keeps its own fresh taskRunId span id. | ||
| def contextRepairedAttempt = new DatabricksParentContext("1234", "5678", "9012", 1) |
There was a problem hiding this comment.
Migrate touched Spock fixture to JUnit 5
The root /workspace/dd-trace-java/AGENTS.md Test frameworks guideline says to use JUnit 5 and migrate an existing Groovy test when it is touched. This change adds repaired-run coverage to the existing Spock fixture instead, so the new coverage remains in the deprecated test framework rather than following the repository instruction.
Useful? React with 👍 / 👎.
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. |
pawel-big-lebowski
left a comment
There was a problem hiding this comment.
Minor comment provided. Overwise looks good.
| return 0; | ||
| } | ||
|
|
||
| private static int indexOf(byte[] haystack, byte[] needle) { |
There was a problem hiding this comment.
Shouldn't the 2 newly added methods land in separate class? If difficult to apply with bytebuddy, please rename indexOf method to something more dbx specific?
There was a problem hiding this comment.
I went for the rename 1d6a64f sounded simpler tbh 🙂
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
What Does This Do
When a Databricks job run is repaired, Databricks reuses the same
jobRunId, so the original and repaired attempts hashed to the same Spark parent trace id and collapsed onto one trace. This mixes the repair attempt index into the parent trace id (suffix-<attempt>, bare for attempt 0) so each attempt lands on its own trace.The attempt index is recovered best-effort from the base64 + java-serialized
unity.scope.datalocal property (keyjobRunAttemptNum); on any failure it falls back to 0, reproducing the previous (non-repair-aware) trace id.Motivation
Correlate Spark spans with the correct Databricks repair-run trace. Mirrors the crawler side in ddoghq/dogweb#411, using the same hash inputs on both sides.
Additional Notes
taskRunId, which Databricks issues fresh per attempt.