Migrate dd-trace-core groovy files to java part 14 - #12332
Conversation
we migrate 3 tests from traceAgentTest module
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.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: 41dae6b7d8
ℹ️ 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".
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
After DDApiIntegrationTest runs locally, JUnit does not run the parent container cleanup. The Docker agent stays active.
🤖 Datadog Autotest · Commit 41dae6b · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
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. |
jpbempel
added
comp: testing
| private static void invokeReport(DefaultDataStreamsMonitoring dataStreams) | ||
| throws ReflectiveOperationException { | ||
| Method report = DefaultDataStreamsMonitoring.class.getDeclaredMethod("report"); | ||
| report.setAccessible(true); | ||
| report.invoke(dataStreams); | ||
| } |
There was a problem hiding this comment.
chore: Annotate DefaultDataStreamsMonitoring.report() with @VisibleForTesting and this can go away.
| invokeReport(dataStreams); | ||
|
|
||
| assertTrue(sharedCommunicationObjects.featuresDiscovery(Config.get()).supportsDataStreams()); | ||
| // conditions.eventually { assert listener.events.size() == 1 } |
There was a problem hiding this comment.
note: Unsure about this comment. Maybe this one can go away ?
| verify(healthMetrics, atLeast(0)).onFailedPublish(anyInt(), anyInt()); | ||
| verifyNoMoreInteractions(healthMetrics); |
There was a problem hiding this comment.
issue: This was spotted by codex and claude. This comment is a bit lengthy because I had to understadn better Spock and Groovy.
tl;dr the suggested change
| verify(healthMetrics, atLeast(0)).onFailedPublish(anyInt(), anyInt()); | |
| verifyNoMoreInteractions(healthMetrics); | |
| verify(healthMetrics, never()).onFailedPublish(anyInt(), anyInt()); | |
| verifyNoMoreInteractions(healthMetrics); |
Explanation:
The original Spock interactions
_ * healthMetrics.onFailedPublish(_)
0 * _
used one _ argument followed by 0 * _. In Spock, _ matches exactly one argument, while the method accepts two.
So any real onFailedPublish(int, int) call was "rejected" by 0 * _.
In the new ported code, the atLeast(0) verification mode explicitly allows to have this invocation without raising an assertion error.
I'm not sure but this may be a slight bug that has been introduced in #4899 thanks to Groovy... So in Groovy tests _ * healthMetrics.onFailedPublish(_) was never matching, the verification fell through the 0 * _.
Given that Java is type safe and the switch to Mockito, the outcome of the ported verification verify(healthMetrics, atLeast(0)).onFailedPublish(anyInt(), anyInt()); actually
relaxes the verification.
claude ran the test locally with verify(healthMetrics, never()).onFailedPublish(anyInt(), anyInt()); and the test was working. I believe the suggestion is more correct in that case and should be applied.
| @TableTest({ | ||
| "scenario | traceCount | lowCardinality | protocol", | ||
| "1 | 0 | true | V1_0 ", | ||
| "2 | 1 | true | V1_0 ", | ||
| "3 | 1 | true | V1_0 ", | ||
| "4 | 2 | true | V1_0 ", | ||
| "5 | 0 | false | V1_0 ", | ||
| "6 | 1 | false | V1_0 ", | ||
| "7 | 1 | false | V1_0 ", | ||
| "8 | 2 | false | V1_0 ", | ||
| "9 | 0 | true | V1_0 ", | ||
| "10 | 1 | true | V1_0 ", | ||
| "11 | 10 | true | V1_0 ", | ||
| "12 | 100 | true | V1_0 ", | ||
| "13 | 0 | false | V1_0 ", | ||
| "14 | 1 | false | V1_0 ", | ||
| "15 | 10 | false | V1_0 ", | ||
| "16 | 100 | false | V1_0 ", | ||
| "17 | 0 | true | V0_5 ", | ||
| "18 | 1 | true | V0_5 ", | ||
| "19 | 1 | true | V0_5 ", | ||
| "20 | 2 | true | V0_5 ", | ||
| "21 | 0 | false | V0_5 ", | ||
| "22 | 1 | false | V0_5 ", | ||
| "23 | 1 | false | V0_5 ", | ||
| "24 | 2 | false | V0_5 ", | ||
| "25 | 0 | true | V0_5 ", | ||
| "26 | 1 | true | V0_5 ", | ||
| "27 | 10 | true | V0_5 ", | ||
| "28 | 100 | true | V0_5 ", | ||
| "29 | 0 | false | V0_5 ", | ||
| "30 | 1 | false | V0_5 ", | ||
| "31 | 10 | false | V0_5 ", | ||
| "32 | 100 | false | V0_5 ", | ||
| "33 | 0 | true | V0_4 ", | ||
| "34 | 1 | true | V0_4 ", | ||
| "35 | 1 | true | V0_4 ", | ||
| "36 | 2 | true | V0_4 ", | ||
| "37 | 0 | false | V0_4 ", | ||
| "38 | 1 | false | V0_4 ", | ||
| "39 | 1 | false | V0_4 ", | ||
| "40 | 2 | false | V0_4 ", | ||
| "41 | 0 | true | V0_4 ", | ||
| "42 | 1 | true | V0_4 ", | ||
| "43 | 10 | true | V0_4 ", | ||
| "44 | 100 | true | V0_4 ", | ||
| "45 | 0 | false | V0_4 ", | ||
| "46 | 1 | false | V0_4 ", | ||
| "47 | 10 | false | V0_4 ", | ||
| "48 | 100 | false | V0_4 " | ||
| }) |
There was a problem hiding this comment.
chore: In Groovy 10 << 10 means (10 * 2¹⁰ = 10 * 1024), given the column name in the Groovy test, I would assume the numbers there are actually sizes, so here 10 << 10 means 10 KiB, 30 << 10 => 30 KiB, and so forth. Here's a suggestion for the scenario names.
For what's it's worth, while I believe my suggestion is more faithful to the original, the actual bufferSize we see in the original groovy test do not appears to be used by this test (even from the f88159f commit that introduced it). Maybe this test can be cleaned up, but this is outside the scope of this PR.
I think adding a small javadoc to explain the reason why the sizes appear in the scenario will be useful for later cleanup.
| @TableTest({ | |
| "scenario | traceCount | lowCardinality | protocol", | |
| "1 | 0 | true | V1_0 ", | |
| "2 | 1 | true | V1_0 ", | |
| "3 | 1 | true | V1_0 ", | |
| "4 | 2 | true | V1_0 ", | |
| "5 | 0 | false | V1_0 ", | |
| "6 | 1 | false | V1_0 ", | |
| "7 | 1 | false | V1_0 ", | |
| "8 | 2 | false | V1_0 ", | |
| "9 | 0 | true | V1_0 ", | |
| "10 | 1 | true | V1_0 ", | |
| "11 | 10 | true | V1_0 ", | |
| "12 | 100 | true | V1_0 ", | |
| "13 | 0 | false | V1_0 ", | |
| "14 | 1 | false | V1_0 ", | |
| "15 | 10 | false | V1_0 ", | |
| "16 | 100 | false | V1_0 ", | |
| "17 | 0 | true | V0_5 ", | |
| "18 | 1 | true | V0_5 ", | |
| "19 | 1 | true | V0_5 ", | |
| "20 | 2 | true | V0_5 ", | |
| "21 | 0 | false | V0_5 ", | |
| "22 | 1 | false | V0_5 ", | |
| "23 | 1 | false | V0_5 ", | |
| "24 | 2 | false | V0_5 ", | |
| "25 | 0 | true | V0_5 ", | |
| "26 | 1 | true | V0_5 ", | |
| "27 | 10 | true | V0_5 ", | |
| "28 | 100 | true | V0_5 ", | |
| "29 | 0 | false | V0_5 ", | |
| "30 | 1 | false | V0_5 ", | |
| "31 | 10 | false | V0_5 ", | |
| "32 | 100 | false | V0_5 ", | |
| "33 | 0 | true | V0_4 ", | |
| "34 | 1 | true | V0_4 ", | |
| "35 | 1 | true | V0_4 ", | |
| "36 | 2 | true | V0_4 ", | |
| "37 | 0 | false | V0_4 ", | |
| "38 | 1 | false | V0_4 ", | |
| "39 | 1 | false | V0_4 ", | |
| "40 | 2 | false | V0_4 ", | |
| "41 | 0 | true | V0_4 ", | |
| "42 | 1 | true | V0_4 ", | |
| "43 | 10 | true | V0_4 ", | |
| "44 | 100 | true | V0_4 ", | |
| "45 | 0 | false | V0_4 ", | |
| "46 | 1 | false | V0_4 ", | |
| "47 | 10 | false | V0_4 ", | |
| "48 | 100 | false | V0_4 " | |
| }) | |
| /** | |
| * The KiB figures in the scenario labels come from the {@code bufferSize} column of the original | |
| * Groovy data table. That column was never read by the test body, so the sizes distinguish the | |
| * rows in reports but do not change what is exercised. | |
| */ | |
| @TableTest({ | |
| "scenario | traceCount | lowCardinality | protocol", | |
| "V1_0, 10 KiB, no traces, low cardinality | 0 | true | V1_0 ", | |
| "V1_0, 10 KiB, 1 trace, low cardinality | 1 | true | V1_0 ", | |
| "V1_0, 30 KiB, 1 trace, low cardinality | 1 | true | V1_0 ", | |
| "V1_0, 30 KiB, 2 traces, low cardinality | 2 | true | V1_0 ", | |
| "V1_0, 10 KiB, no traces, high cardinality | 0 | false | V1_0 ", | |
| "V1_0, 10 KiB, 1 trace, high cardinality | 1 | false | V1_0 ", | |
| "V1_0, 30 KiB, 1 trace, high cardinality | 1 | false | V1_0 ", | |
| "V1_0, 30 KiB, 2 traces, high cardinality | 2 | false | V1_0 ", | |
| "V1_0, 100 KiB, no traces, low cardinality | 0 | true | V1_0 ", | |
| "V1_0, 100 KiB, 1 trace, low cardinality | 1 | true | V1_0 ", | |
| "V1_0, 100 KiB, 10 traces, low cardinality | 10 | true | V1_0 ", | |
| "V1_0, 100 KiB, 100 traces, low cardinality | 100 | true | V1_0 ", | |
| "V1_0, 100 KiB, no traces, high cardinality | 0 | false | V1_0 ", | |
| "V1_0, 100 KiB, 1 trace, high cardinality | 1 | false | V1_0 ", | |
| "V1_0, 100 KiB, 10 traces, high cardinality | 10 | false | V1_0 ", | |
| "V1_0, 100 KiB, 100 traces, high cardinality | 100 | false | V1_0 ", | |
| "V0_5, 10 KiB, no traces, low cardinality | 0 | true | V0_5 ", | |
| "V0_5, 10 KiB, 1 trace, low cardinality | 1 | true | V0_5 ", | |
| "V0_5, 30 KiB, 1 trace, low cardinality | 1 | true | V0_5 ", | |
| "V0_5, 30 KiB, 2 traces, low cardinality | 2 | true | V0_5 ", | |
| "V0_5, 10 KiB, no traces, high cardinality | 0 | false | V0_5 ", | |
| "V0_5, 10 KiB, 1 trace, high cardinality | 1 | false | V0_5 ", | |
| "V0_5, 30 KiB, 1 trace, high cardinality | 1 | false | V0_5 ", | |
| "V0_5, 30 KiB, 2 traces, high cardinality | 2 | false | V0_5 ", | |
| "V0_5, 100 KiB, no traces, low cardinality | 0 | true | V0_5 ", | |
| "V0_5, 100 KiB, 1 trace, low cardinality | 1 | true | V0_5 ", | |
| "V0_5, 100 KiB, 10 traces, low cardinality | 10 | true | V0_5 ", | |
| "V0_5, 100 KiB, 100 traces, low cardinality | 100 | true | V0_5 ", | |
| "V0_5, 100 KiB, no traces, high cardinality | 0 | false | V0_5 ", | |
| "V0_5, 100 KiB, 1 trace, high cardinality | 1 | false | V0_5 ", | |
| "V0_5, 100 KiB, 10 traces, high cardinality | 10 | false | V0_5 ", | |
| "V0_5, 100 KiB, 100 traces, high cardinality | 100 | false | V0_5 ", | |
| "V0_4, 10 KiB, no traces, low cardinality | 0 | true | V0_4 ", | |
| "V0_4, 10 KiB, 1 trace, low cardinality | 1 | true | V0_4 ", | |
| "V0_4, 30 KiB, 1 trace, low cardinality | 1 | true | V0_4 ", | |
| "V0_4, 30 KiB, 2 traces, low cardinality | 2 | true | V0_4 ", | |
| "V0_4, 10 KiB, no traces, high cardinality | 0 | false | V0_4 ", | |
| "V0_4, 10 KiB, 1 trace, high cardinality | 1 | false | V0_4 ", | |
| "V0_4, 30 KiB, 1 trace, high cardinality | 1 | false | V0_4 ", | |
| "V0_4, 30 KiB, 2 traces, high cardinality | 2 | false | V0_4 ", | |
| "V0_4, 100 KiB, no traces, low cardinality | 0 | true | V0_4 ", | |
| "V0_4, 100 KiB, 1 trace, low cardinality | 1 | true | V0_4 ", | |
| "V0_4, 100 KiB, 10 traces, low cardinality | 10 | true | V0_4 ", | |
| "V0_4, 100 KiB, 100 traces, low cardinality | 100 | true | V0_4 ", | |
| "V0_4, 100 KiB, no traces, high cardinality | 0 | false | V0_4 ", | |
| "V0_4, 100 KiB, 1 trace, high cardinality | 1 | false | V0_4 ", | |
| "V0_4, 100 KiB, 10 traces, high cardinality | 10 | false | V0_4 ", | |
| "V0_4, 100 KiB, 100 traces, high cardinality | 100 | false | V0_4 " | |
| }) |
| assertInstanceOf(Map.class, agentResponse.get().get("rate_by_service")); | ||
| } | ||
|
|
||
| // spotless:off |
There was a problem hiding this comment.
nitpick: Is the spotless:off useful here ?
It looks it was present on the groovy counterpart, but this doesn't seem to be needed here.
This looks to be the case for other methods.
|
|
||
| RemoteApi.Response response = | ||
| api.sendSerializedTraces( | ||
| prepareRequest(Collections.singletonList(Collections.singletonList(span)), mapper)); |
There was a problem hiding this comment.
chore: Simplification (also needs the static import)
| prepareRequest(Collections.singletonList(Collections.singletonList(span)), mapper)); | |
| prepareRequest(singletonList(span), mapper)); |
| unixDomainSocketApi.sendSerializedTraces( | ||
| prepareRequest(Collections.<List<DDSpan>>emptyList(), mapper)); |
There was a problem hiding this comment.
nitpick: Maybe use a static import
| unixDomainSocketApi.sendSerializedTraces( | |
| prepareRequest(Collections.<List<DDSpan>>emptyList(), mapper)); | |
| unixDomainSocketApi.sendSerializedTraces( | |
| prepareRequest(emptyList(), mapper)); |
|
|
||
| RemoteApi.Response response = | ||
| unixDomainSocketApi.sendSerializedTraces( | ||
| prepareRequest(Collections.singletonList(Collections.singletonList(span)), mapper)); |
There was a problem hiding this comment.
chore: Simplification
| prepareRequest(Collections.singletonList(Collections.singletonList(span)), mapper)); | |
| prepareRequest(singletonList(span), mapper)); |
| ControllableTimeSource timeSource = new ControllableTimeSource(); | ||
|
|
||
| TraceConfig traceConfig = mock(TraceConfig.class); | ||
| when(traceConfig.isDataStreamsEnabled()).thenReturn(true); | ||
|
|
||
| try (DefaultDataStreamsMonitoring dataStreams = | ||
| new DefaultDataStreamsMonitoring( | ||
| sink, | ||
| sharedCommunicationObjects.featuresDiscovery(Config.get()), | ||
| timeSource, | ||
| () -> traceConfig, | ||
| Config.get())) { | ||
|
|
||
| dataStreams.start(); | ||
| DataStreamsTags tags = | ||
| DataStreamsTags.create("testType", null, "testTopic", "testGroup", null); | ||
| dataStreams.add( | ||
| new StatsPoint(tags, 1, 2, 5, timeSource.getCurrentTimeNanos(), 0, 0, 0, null)); | ||
| timeSource.advance(Config.get().getDataStreamsBucketDurationNanoseconds()); | ||
| invokeReport(dataStreams); | ||
|
|
||
| assertTrue(sharedCommunicationObjects.featuresDiscovery(Config.get()).supportsDataStreams()); |
There was a problem hiding this comment.
chore: Make sharedCommunicationObjects.featuresDiscovery(Config.get()) a local var
| ControllableTimeSource timeSource = new ControllableTimeSource(); | |
| TraceConfig traceConfig = mock(TraceConfig.class); | |
| when(traceConfig.isDataStreamsEnabled()).thenReturn(true); | |
| try (DefaultDataStreamsMonitoring dataStreams = | |
| new DefaultDataStreamsMonitoring( | |
| sink, | |
| sharedCommunicationObjects.featuresDiscovery(Config.get()), | |
| timeSource, | |
| () -> traceConfig, | |
| Config.get())) { | |
| dataStreams.start(); | |
| DataStreamsTags tags = | |
| DataStreamsTags.create("testType", null, "testTopic", "testGroup", null); | |
| dataStreams.add( | |
| new StatsPoint(tags, 1, 2, 5, timeSource.getCurrentTimeNanos(), 0, 0, 0, null)); | |
| timeSource.advance(Config.get().getDataStreamsBucketDurationNanoseconds()); | |
| invokeReport(dataStreams); | |
| assertTrue(sharedCommunicationObjects.featuresDiscovery(Config.get()).supportsDataStreams()); | |
| ControllableTimeSource timeSource = new ControllableTimeSource(); | |
| DDAgentFeaturesDiscovery featuresDsicovery = sharedCommunicationObjects.featuresDiscovery(Config.get()); | |
| TraceConfig traceConfig = mock(TraceConfig.class); | |
| when(traceConfig.isDataStreamsEnabled()).thenReturn(true); | |
| try (DefaultDataStreamsMonitoring dataStreams = | |
| new DefaultDataStreamsMonitoring( | |
| sink, | |
| featuresDiscovery, | |
| timeSource, | |
| () -> traceConfig, | |
| Config.get())) { | |
| dataStreams.start(); | |
| DataStreamsTags tags = | |
| DataStreamsTags.create("testType", null, "testTopic", "testGroup", null); | |
| dataStreams.add( | |
| new StatsPoint(tags, 1, 2, 5, timeSource.getCurrentTimeNanos(), 0, 0, 0, null)); | |
| timeSource.advance(Config.get().getDataStreamsBucketDurationNanoseconds()); | |
| invokeReport(dataStreams); | |
| assertTrue(featuresDiscovery.supportsDataStreams()); |
4 participants
What Does This Do
we migrate 3 tests from traceAgentTest module
Motivation
this is part of the effort to migrate groovy tests to Java/JUnit
part1: #11053
part2: #11062
part3: #11085
part4: #11146
part5: #11217
part6: #11362
part7: #11374
part8: #11437
part9: #11488
part10: #11543
part11: #11566
part12: #11619
part13: #12253
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: [PROJ-IDENT]