Port TraceMapperV0_4/V0_5 payload tests from Spock to JUnit 5 - #11871
Conversation
Migrate TraceMapperV04PayloadTest and TraceMapperV05PayloadTest from Groovy/Spock to JUnit 5 Java (V0.1/V1 deferred). The two `where:` tables become @MethodSource providers (preserving the `10<<10` shift intent), the V0.4 metaStruct closure becomes a @FunctionalInterface, and the format-agnostic decode helpers (assertEqualsWithNullAsEmpty, the metrics unpackNumber switch, and the growable capturing channel) are extracted to a shared PayloadVerifiers helper reused by both. The two format-specific PayloadVerifiers stay per-test since V0.4 (inline strings) and V0.5 (dictionary-compressed indices) decode different wire formats. Two behaviors DDSpecification/Groovy provided implicitly are made explicit: (1) numeric metrics are compared via longValue() — Groovy's dynamic dispatch routed assertEquals(Long,Integer) to the primitive long overload, whereas Java's static resolution would compare by reference type after msgpack's minimal encoding narrows a Long to an Integer; (2) the ProcessTags static is re-synced to the per-test Config via @ExtendWith(WithConfigExtension) + a @beforeeach, with the overflow test disabling process tags via @WithConfig. Test count is unchanged (V0.4=23, V0.5=30). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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. |
|
Bits Code status: ✅ Done Comment @DataDog to request changes |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a68d1ce53
ℹ️ 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".
| import datadog.trace.junit.utils.config.WithConfig; | ||
| import datadog.trace.junit.utils.config.WithConfigExtension; |
There was a problem hiding this comment.
Import the existing WithConfig test utilities
A repo-wide search shows WithConfig and WithConfigExtension are provided under datadog.trace.test.junit.utils.config, not datadog.trace.junit.utils.config. As written, these imports leave both migrated test classes with unresolved symbols and break dd-trace-core test compilation until the imports are changed to the actual package.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 2c7643e. The utilities live under datadog.trace.test.junit.utils.config and my migration used the old package. 👍
| // disable process tags since they are only on the first span of the chunk otherwise the | ||
| // calculation woes | ||
| @Test | ||
| @WithConfig(key = EXPERIMENTAL_PROPAGATE_PROCESS_TAGS_ENABLED, value = "false") |
There was a problem hiding this comment.
Reset process tags after disabling them
This test now disables process-tag propagation through @WithConfig, and the class @BeforeEach syncs the ProcessTags static to that disabled Config; however, WithConfigExtension only rebuilds Config after the method and does not reset ProcessTags. If this method is the last one run in the class, later tests in the same Gradle test JVM can observe process tags still disabled, whereas the original Spock cleanup explicitly re-enabled the config and reset ProcessTags.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This seems accurate, albeit unlikely. Still worth a fix IMO.
There was a problem hiding this comment.
Agreed. Added an @AfterAll ProcessTags.reset(Config.get()) in 2c7643e, matching DDSpanSerializationTest — by afterAll the extension has already restored Config to its defaults, so this resets the static and prevents the disabled state leaking across the shared test JVM. 👍
There was a problem hiding this comment.
Fixed in 2c7643e — added an @AfterAll ProcessTags.reset(Config.get()), mirroring DDSpanSerializationTest. By afterAll the extension has restored Config to defaults, so the static gets reset and can't leak into other tests in the shared JVM.
| import datadog.trace.common.writer.Payload; | ||
| import datadog.trace.common.writer.TraceGenerator.PojoSpan; | ||
| import datadog.trace.core.DDSpanContext; | ||
| import datadog.trace.junit.utils.config.WithConfigExtension; |
There was a problem hiding this comment.
| import datadog.trace.junit.utils.config.WithConfigExtension; | |
| import datadog.trace.test.junit.utils.config.WithConfigExtension; |
| import datadog.trace.common.writer.TraceGenerator.PojoSpan; | ||
| import datadog.trace.core.DDSpanContext; | ||
| import datadog.trace.junit.utils.config.WithConfig; | ||
| import datadog.trace.junit.utils.config.WithConfigExtension; |
There was a problem hiding this comment.
| import datadog.trace.junit.utils.config.WithConfigExtension; | |
| import datadog.trace.test.junit.utils.config.WithConfigExtension; |
| // disable process tags since they are only on the first span of the chunk otherwise the | ||
| // calculation woes | ||
| @Test | ||
| @WithConfig(key = EXPERIMENTAL_PROPAGATE_PROCESS_TAGS_ENABLED, value = "false") |
There was a problem hiding this comment.
This seems accurate, albeit unlikely. Still worth a fix IMO.
The migrated tests imported datadog.trace.junit.utils.config.* but the utilities live in datadog.trace.test.junit.utils.config.* — the wrong package broke dd-trace-core test compilation. Correct the imports in both V0.4 and V0.5 payload tests. V0.5 disables process-tag propagation via a method-level @WithConfig. WithConfigExtension rebuilds Config after each test but does not reset the ProcessTags static, so a disabled state could leak into other tests in the same Gradle JVM (the original Spock cleanup reset it explicitly). Add an @afterall ProcessTags.reset, mirroring DDSpanSerializationTest. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
What Does This Do
Migrates
TraceMapperV04PayloadTestandTraceMapperV05PayloadTestfrom Groovy/Spock to JUnit 5 Java.TraceMapperV1PayloadTestandTraceMapperTestare left untouched (V1 is newer and not yet in production — deferred).Motivation
Moving away from Groovy - wanted to clean-up before doing further optimizations in v04 & v05 serialization code
Additional Notes
From Claude...
where:tables →@MethodSourceproviders, preserving the10<<10/100<<10shift intent (not flattened to decimals).metaStructverifier closure → a@FunctionalInterface MetaStructVerifier<E>.assertEqualsWithNullAsEmpty, the metricsunpackNumberswitch, and the growable capturingWritableByteChannel— are extracted to a sharedPayloadVerifiershelper reused by both tests. The twoPayloadVerifiers themselves stay per-test because V0.4 (inline strings) and V0.5 (dictionary-compressed string indices) decode different wire formats.Two behaviors that
DDSpecification/ Groovy provided implicitly are now explicit:longValue(). Groovy's dynamic dispatch routedassertEquals(Long, Integer)to the primitiveassertEquals(long, long)overload (numeric equality); Java's static overload resolution would pick theObjectoverload and compare by reference type, which fails once msgpack's minimal encoding narrows aLongthread id back to anInteger.ProcessTagsstatic is re-synced to the per-testConfigvia@ExtendWith(WithConfigExtension.class)+ a@BeforeEach, and the body-overflow test disables process tags with@WithConfig(so the dictionary-size calculation stays stable).Testing
Test count is unchanged: V0.4 = 23, V0.5 = 30 (53 total), all green. Verified the original Groovy suite passes in the same environment before porting.
🤖 Generated with Claude Code