Uh oh!
There was an error while loading. Please reload this page.
Migrate OpenTelemetry smoke test to JUnit - #12127
Conversation
There was a problem hiding this comment.
More details
The JUnit migration preserves the OpenTelemetry smoke application launch and validates its one annotated trace plus ten sequential manual traces with the new backend assertions. No diff-only behavioral regression was confirmed; no additional tests recommended because the existing application shape is deterministic and the attempted module execution was blocked by unavailable Gradle/JDK tooling.
📊 Validated against 3 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit 9eece4a · What is Autotest? · @DataDog review to ask questions · 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:9eece4ad18
ℹ️ 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".
Uh oh!
There was an error while loading. Please reload this page.
🟢 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. |
e3bc1dc to
f9175c8Compare🎯 Code Coverage (details) 🔗 Commit SHA: bb1762b | Docs | View more details | Give us feedback! |
f9175c8 to
36e7755CompareBits has a CI fix ready🟢 Investigated · 🟢 Fix prepared · ⚪ Validation skipped · 🟠 Ready
View in Datadog | Reviewed commit 36e7755 · Any feedback? Reach out in #deveng-pr-agent |
13ba721 to
97b671fCompare97b671f to
54741a2Compare54741a2 to
5eaac9aCompare5eaac9a to
7f045dfCompare| trace(span().root().operationName("Application.annotatedSpan")), | ||
| trace(span().root().resourceName("span-0")), | ||
| trace(span().root().resourceName("span-1")), | ||
| trace(span().root().resourceName("span-2")), | ||
| trace(span().root().resourceName("span-3")), | ||
| trace(span().root().resourceName("span-4")), | ||
| trace(span().root().resourceName("span-5")), | ||
| trace(span().root().resourceName("span-6")), | ||
| trace(span().root().resourceName("span-7")), | ||
| trace(span().root().resourceName("span-8")), | ||
| trace(span().root().resourceName("span-9"))); |
There was a problem hiding this comment.
nit: Looks like a copy-paste
Can we think of some more elegant checks?
There was a problem hiding this comment.
Indeed, in the groovy equivalent it was waitForTraceCount(11), but this method's use is a tad pervasive, I found 152 callsites.
Also there's a Traces::waitForTraceCount method, shouldn't it be used ?
There was a problem hiding this comment.
Can we think of some more elegant checks?
This come from the trace structure that is useless and not representing anything meanful.
The goal would be more to exercice the OTel API to build meaningful traces, which should give more elegant assert rules.
But it feels out of scope for this PR. The PR now checks the resource names, which is still a win compared to tho original trace count only.
Also there's a Traces::waitForTraceCount method, shouldn't it be used ?
The goal is to move to stricter trace structure testing. Trace count won't check for the format.
I expect to refine the test app behavior to something more meaningful so I only have basic test asserts for now.
| com.fasterxml.jackson.core:jackson-annotations:2.10.3=testCompileClasspath,testRuntimeClasspath | ||
| com.github.docker-java:docker-java-api:3.4.2=testCompileClasspath,testRuntimeClasspath | ||
| com.github.docker-java:docker-java-transport-zerodep:3.4.2=testCompileClasspath,testRuntimeClasspath | ||
| com.github.docker-java:docker-java-transport:3.4.2=testCompileClasspath,testRuntimeClasspath |
There was a problem hiding this comment.
Just curious why lock file changed?
There was a problem hiding this comment.
This might come from the addition of test containers. I regenerated and the content seems up-to-date.
bric3
left a comment
There was a problem hiding this comment.
Looks good to me, however I think the comment on assertTrace and the list of spans deserves some attention.
| trace(span().root().operationName("Application.annotatedSpan")), | ||
| trace(span().root().resourceName("span-0")), | ||
| trace(span().root().resourceName("span-1")), | ||
| trace(span().root().resourceName("span-2")), | ||
| trace(span().root().resourceName("span-3")), | ||
| trace(span().root().resourceName("span-4")), | ||
| trace(span().root().resourceName("span-5")), | ||
| trace(span().root().resourceName("span-6")), | ||
| trace(span().root().resourceName("span-7")), | ||
| trace(span().root().resourceName("span-8")), | ||
| trace(span().root().resourceName("span-9"))); |
There was a problem hiding this comment.
Indeed, in the groovy equivalent it was waitForTraceCount(11), but this method's use is a tad pervasive, I found 152 callsites.
Also there's a Traces::waitForTraceCount method, shouldn't it be used ?
7f045df to
8808628CompareThere was a problem hiding this comment.
More details
The JUnit test keeps the prior check for 11 traces and exit value 0. It also checks each trace and span with the new smoke test framework.
🤖 Datadog Autotest · Commit 8808628 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
8808628 to
b88ff9eCompareb88ff9e to
921c1ccCompare921c1cc to
24d9a52Compare24d9a52 to
bb1762bCompareThere was a problem hiding this comment.
More details
The JUnit migration preserves the 11 expected OpenTelemetry traces and the zero exit code check. The shared smoke-test framework has no reportable static correctness issue.
🤖 Datadog Autotest · Commit bb1762b · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
What Does This Do
This PR migrates the OpenTelemetry smoke tests to the new JUnit smoke test framework.
Motivation
The new tests checks the whole trace structures and span values, in addition to trace payload validity and invariants.
Additional Notes
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