Migrate dd-trace-core groovy files to java part 12 - #11619
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. |
|
🎯 Code Coverage (details) 🔗 Commit SHA: caa69aa | Docs | View more details | Give us feedback! |
🟢 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. |
| private static void assertEqualsWithNullAsEmpty(CharSequence expected, CharSequence actual) { | ||
| if (null == expected) { | ||
| assertEquals("", actual); | ||
| } else { | ||
| assertEquals(expected.toString(), actual.toString()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Would it make sense to use ? operator instead?
And also, I'm a bit skeptical about Yoda style in general, maybe make sense to update skill to not use it for migrated code?
| private static void assertEqualsWithNullAsEmpty(CharSequence expected, CharSequence actual) { | |
| if (null == expected) { | |
| assertEquals("", actual); | |
| } else { | |
| assertEquals(expected.toString(), actual.toString()); | |
| } | |
| } | |
| private static void assertEqualsWithNullAsEmpty(CharSequence expected, CharSequence actual) { | |
| assertEquals(expected == null ? "" : expected.toString(), actual.toString()); | |
| } |
There was a problem hiding this comment.
done
agree on yoda conditions, totally useless in Java. some people use this style inherited from C/C++ 🤷
| // disable process tags since they are only on the first span of the chunk otherwise the | ||
| // calculation woes | ||
| // 4x 36 ASCII characters and 2 bytes of msgpack string prefix |
There was a problem hiding this comment.
nit: can be 2 lines of comments
| // calculation woes | ||
| // 4x 36 ASCII characters and 2 bytes of msgpack string prefix | ||
| int dictionarySpacePerTrace = 4 * (36 + 2); | ||
| // enough space for two traces with distinct string values, plus the header |
There was a problem hiding this comment.
Just an idea, maybe worth to add into skill to write comments as normal sentences?
With Capital letter first word, and . at the end...
| // enough space for two traces with distinct string values, plus the header | |
| // Enough space for two traces with distinct string values, plus the header. |
There was a problem hiding this comment.
we really care about this, you can add it to the skill. personally I don't mind.
| for (int k = 0; k < spanCount; ++k) { | ||
| TraceGenerator.PojoSpan expectedSpan = expectedTrace.get(k); | ||
| int elementCount = unpacker.unpackArrayHeader(); | ||
| assertEquals(12, elementCount); | ||
| String serviceName = dictionary[unpacker.unpackInt()]; | ||
| assertEqualsWithNullAsEmpty(expectedSpan.getServiceName(), serviceName); | ||
| String operationName = dictionary[unpacker.unpackInt()]; | ||
| assertEqualsWithNullAsEmpty(expectedSpan.getOperationName(), operationName); | ||
| String resourceName = dictionary[unpacker.unpackInt()]; | ||
| assertEqualsWithNullAsEmpty(expectedSpan.getResourceName(), resourceName); | ||
| long traceId = unpacker.unpackValue().asNumberValue().toLong(); | ||
| assertEquals(expectedSpan.getTraceId().toLong(), traceId); | ||
| long spanId = unpacker.unpackValue().asNumberValue().toLong(); | ||
| assertEquals(expectedSpan.getSpanId(), spanId); | ||
| long parentId = unpacker.unpackValue().asNumberValue().toLong(); | ||
| assertEquals(expectedSpan.getParentId(), parentId); | ||
| long startTime = unpacker.unpackLong(); | ||
| assertEquals(expectedSpan.getStartTime(), startTime); | ||
| long duration = unpacker.unpackLong(); | ||
| assertEquals(expectedSpan.getDurationNano(), duration); | ||
| int error = unpacker.unpackInt(); | ||
| assertEquals(expectedSpan.getError(), error); | ||
| int metaSize = unpacker.unpackMapHeader(); | ||
| HashMap<String, String> meta = new HashMap<>(); | ||
| for (int j = 0; j < metaSize; ++j) { | ||
| meta.put(dictionary[unpacker.unpackInt()], dictionary[unpacker.unpackInt()]); | ||
| } |
There was a problem hiding this comment.
nit: Also an idea if we can instruct skill to apply better readability here?
Adding empty lines between asserts helps to read code better.
like:
TraceGenerator.PojoSpan expectedSpan = expectedTrace.get(k);
int elementCount = unpacker.unpackArrayHeader();
assertEquals(12, elementCount);
String serviceName = dictionary[unpacker.unpackInt()];
assertEqualsWithNullAsEmpty(expectedSpan.getServiceName(), serviceName);
String operationName = dictionary[unpacker.unpackInt()];
assertEqualsWithNullAsEmpty(expectedSpan.getOperationName(), operationName);
String resourceName = dictionary[unpacker.unpackInt()];
assertEqualsWithNullAsEmpty(expectedSpan.getResourceName(), resourceName);
...
There was a problem hiding this comment.
I personally prefer avoid empty lines. I feel that adding this to the skill will result in weird behavior.
| if (Tags.HTTP_STATUS.equals(entry.getKey())) { | ||
| assertEquals(String.valueOf(expectedSpan.getHttpStatusCode()), entry.getValue()); | ||
| } else if (DDTags.ORIGIN_KEY.equals(entry.getKey())) { | ||
| assertEquals(expectedSpan.getOrigin(), entry.getValue()); | ||
| } else if (DDTags.PROCESS_TAGS.equals(entry.getKey())) { | ||
| processTagsCount++; | ||
| assertTrue(Config.get().isExperimentalPropagateProcessTagsEnabled()); | ||
| assertEquals(0, k); | ||
| assertEquals(ProcessTags.getTagsForSerialization().toString(), entry.getValue()); | ||
| } else { | ||
| Object tag = expectedSpan.getTag(entry.getKey()); |
There was a problem hiding this comment.
Looks like switch can be used here?
| boolean ciVisibilityEnabled, | ||
| boolean ciVisibilityAgentlessEnabled, |
There was a problem hiding this comment.
nit: ciVis prefix would be shorter and more readable, IMHO.
|
|
||
| HealthMetrics monitor = mock(HealthMetrics.class); | ||
| TraceProcessingWorker worker = mock(TraceProcessingWorker.class); | ||
| DDAgentFeaturesDiscovery discovery = mock(DDAgentFeaturesDiscovery.class); | ||
| DDAgentApi api = mock(DDAgentApi.class); | ||
| MonitoringImpl monitoring = new MonitoringImpl(StatsDClient.NO_OP, 1, TimeUnit.SECONDS); | ||
| PayloadDispatcherImpl dispatcher = | ||
| new PayloadDispatcherImpl(new DDAgentMapperDiscovery(discovery), api, monitor, monitoring); | ||
| DDAgentWriter writer = new DDAgentWriter(worker, dispatcher, monitor, 1, TimeUnit.SECONDS, false); | ||
|
|
||
| // Only used to create spans | ||
| CoreTracer dummyTracer; |
There was a problem hiding this comment.
nit: Not sure why but looks like skill tends to not use private + final (where applicable). I saw such code several times in previous PRs. Probably worth to add to skill?
There was a problem hiding this comment.
honestly don't care for test + it's shorter
There was a problem hiding this comment.
Yep, but it is some sort of habit for me, like I feel pain when I see code written in Python-all-global style :)
Just kidding...
| @AfterEach | ||
| void cleanup() { | ||
| writer.close(); | ||
| if (dummyTracer != null) { |
There was a problem hiding this comment.
nit: Why we need check for != null if we create it in setup()?
| TraceProcessingWorker worker = mock(TraceProcessingWorker.class); | ||
| DDAgentFeaturesDiscovery discovery = mock(DDAgentFeaturesDiscovery.class); | ||
| DDAgentApi api = mock(DDAgentApi.class); | ||
| MonitoringImpl monitoring = new MonitoringImpl(StatsDClient.NO_OP, 1, TimeUnit.SECONDS); |
There was a problem hiding this comment.
nit: I would use static import for TimeUnit.SECONDS.
| new DDAgentWriter(localWorker, localDispatcher, localMonitor, 1, TimeUnit.SECONDS, false); | ||
|
|
||
| DDSpan p0 = newSpan(); | ||
| List<DDSpan> trace = java.util.Arrays.asList(p0, newSpan()); |
There was a problem hiding this comment.
nit: FQN required here?
| DDSpan p0 = newSpan(); | ||
| List<DDSpan> trace = java.util.Arrays.asList(p0, newSpan()); |
There was a problem hiding this comment.
This code looks a bit confusing. Why one span created as variable and second as inlined call? Here and similar places.
How about just: List<DDSpan> trace = asList(newSpan(), newSpan()); ?
Found in Google:
Java will never reorder the execution of method calls passed as arguments to another method. The Java Language Specification (JLS) Section 15.7 strictly guarantees that all expressions and arguments are evaluated from left to right.
|
|
||
| DDSpan newSpan() { | ||
| // Use the UNSET-priority variant so setSamplingPriority() can change the priority later | ||
| return buildSpan(0L, "test.tag", "test.value", PropagationTags.factory().empty()); |
There was a problem hiding this comment.
nit: why not use PrioritySampling.UNSET instead of 0L?
There was a problem hiding this comment.
because here 0L is the timestamp not the priority
9cf428e to
c71437c
Compare
PerfectSlayer
left a comment
There was a problem hiding this comment.
I will do a review by the end of the week. Can you hold it until then if already approved?
I would like to extract feedback from my manual changes and see if anything can relate to those changes (additionally to review them manually).
| } finally { | ||
| agent.close(); | ||
| } |
There was a problem hiding this comment.
nitpick: I think this can be quickly improved everywhere as a try-with-resources as agent is an AutoCloseable type.
bric3
left a comment
There was a problem hiding this comment.
I think we can move forward.
I think I'd prefer to use the try-with-resources where applicable as part of the port though.
|
Starting the review 👀 |
|
❔ question: Can I push changes from the automatic review I created in #11636 or would you prefer to paste all findings as comments? |
PerfectSlayer
left a comment
There was a problem hiding this comment.
Flushing comments for dd-trace-core/src/test/java/datadog/trace/common/writer/ddagent/TraceMapperV04PayloadTest.java.
It's going to be a long review. I wonder if we should not split it and pair with the original author so they could fix the issues before considering merging this test implementation.
| class TraceMapperV04PayloadTest extends DDJavaSpecification { | ||
|
|
||
| @TableTest({ | ||
| "scenario | bufferSize | traceCount | lowCardinality", |
There was a problem hiding this comment.
🔨 issue: buffer size were migrated to magic numbers. Better use MethodSource and build arguments so we can keep the meaning.
| arguments(DD64bTraceId.ONE, 2L, 3L), | ||
| arguments(DD64bTraceId.MAX, 2L, 3L), | ||
| arguments(DD64bTraceId.from(-10), -11L, -12L)); |
There was a problem hiding this comment.
🔨 issue: This is a trace id converter for 1 and MAX at least
| Collections.emptyMap(), | ||
| Collections.emptyMap(), | ||
| "type", | ||
| false, | ||
| 0, | ||
| 0, | ||
| "origin"); | ||
| List<List<TraceGenerator.PojoSpan>> traces = | ||
| Collections.singletonList(Collections.singletonList(span)); |
There was a problem hiding this comment.
🎯 suggestion: Would be easier to read if we use static import for emptyMap() and singletonList()
| traceMapper, | ||
| (expectedObj, received) -> { | ||
| List<?> expected = (List<?>) expectedObj; | ||
| MessageUnpacker unpacker = MessagePack.newDefaultUnpacker(received); |
There was a problem hiding this comment.
💭 thought: The packer is never close. Maybe use try-with-resources?
| traces, | ||
| traceMapper, | ||
| (expectedObj, received) -> { | ||
| List<?> expected = (List<?>) expectedObj; |
There was a problem hiding this comment.
🎯 suggestion: Check type first?
| "origin")); | ||
| } | ||
|
|
||
| List<List<TraceGenerator.PojoSpan>> traces = Collections.singletonList(spans); |
There was a problem hiding this comment.
💭 thought: That will be nice the day we get var or an equivalent syntactic sugar :)
| // --- Inner classes --- | ||
|
|
||
| private interface MetaStructVerifier { | ||
| void verify(Object expected, byte[] received) throws IOException; |
There was a problem hiding this comment.
❔ question: It loosed the generic type. Should we keep it or improve it? Because there is only 1 usage in this test file.
| Payload payload = mapper.newPayload().withBody(messageCount, buffer); | ||
| payload.writeTo(this); | ||
| captured.flip(); | ||
| MessageUnpacker unpacker = MessagePack.newDefaultUnpacker(captured); |
There was a problem hiding this comment.
🔨 issue: Same here, unpacker not closed.
| if (DD_MEASURED.toString().equals(key)) { | ||
| assertTrue(metricValue.intValue() == 1 || !expectedSpan.isMeasured()); |
There was a problem hiding this comment.
🎯 suggestion: The assertion around span measured is confusing and can be simplified.
| for (Map.Entry<String, Number> metric : metrics.entrySet()) { | ||
| if (metric.getValue() instanceof Double || metric.getValue() instanceof Float) { |
There was a problem hiding this comment.
🎯 suggestion: Poor readability. metrics.forEach() would improve it a bit at least.
2cc67b3 to
35a718a
Compare
4ea79e9 to
8796f31
Compare
4d30733 to
e120ab9
Compare
There was a problem hiding this comment.
Several migrated tests accept wrong metric arguments or omit required negative checks. The dropped-trace test also uses an unset priority, so it no longer checks the dropped-priority path.
🤖 Datadog Autotest · Commit 9d23634 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| DDSpan p0 = newSpan(); | ||
| List<DDSpan> trace = java.util.Arrays.asList(p0, newSpan()); | ||
|
|
||
| when(worker.publish(eq(trace.get(0)), anyInt(), eq(trace))).thenReturn(publishResult); |
There was a problem hiding this comment.
A wrong sampling priority in the intake writer can pass this test.
Assertion details
- Input: Run testDroppedTraceIsCounted with either dropped publish result.
- Expected:
The migrated test must send SAMPLER_DROP to worker.publish, as the former test does. - Actual:
The span keeps the UNSET priority. The anyInt() matcher accepts this value. Set p0 to SAMPLER_DROP and require that value in both publish checks.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| return null; | ||
| }) | ||
| .when(healthMetrics) | ||
| .onFailedSerialize(any(), any()); |
There was a problem hiding this comment.
The worker can replace the serialization error without a test failure.
Assertion details
- Input: PayloadDispatcherImpl.addTrace throws theError during trace processing.
- Expected:
The health metric callback must receive the exact exception from addTrace. - Actual:
The any() matcher accepts a different throwable. Use the same theError object in the callback matcher and keep the asynchronous counter.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| if (expectedSubmitted == acceptedCount.get()) break; | ||
| Thread.sleep(50); | ||
| } | ||
| assertEquals(expectedSubmitted, acceptedCount.get()); |
There was a problem hiding this comment.
An extra serialization failure report can pass this test.
Assertion details
- Input: Process any table row in testTracesShouldBeProcessed.
- Expected:
Accepted traces dispatch without a serialization failure metric. - Actual:
The test checks only the dispatch count. Add a negative check for healthMetrics.onFailedSerialize after processing completes.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
|
|
||
| // monitor is notified of successful publication | ||
| verify(worker).publish(any(), anyInt(), eq(trace)); | ||
| verify(monitor).onPublish(any(), anyInt()); |
There was a problem hiding this comment.
A writer can report metrics for the wrong trace without a test failure.
Assertion details
- Input: A writer publishes one trace successfully.
- Expected:
onPublish must receive the same trace that writer.write receives. - Actual:
The any() matcher accepts a different trace. Restore the exact trace matcher in this test and in the equivalent intake and combined writer tests.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| verify(healthMetrics, times(1)).onFlush(false); | ||
| verify(healthMetrics, times(1)) | ||
| .onSend( | ||
| anyInt(), |
There was a problem hiding this comment.
Wrong trace-count metrics can pass the migrated tests.
Assertion details
- Input: The monitor test writes and sends one trace.
- Expected:
onSend and onFailedSend must report one trace for these requests. - Actual:
The first anyInt() matcher accepts a wrong trace count. Require 1 in the agent and intake happy-path and error monitor tests.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
9d23634 to
b83b8cc
Compare
| JavaTestHttpServer intake = | ||
| JavaTestHttpServer.httpServer( | ||
| s -> | ||
| s.handlers( | ||
| h -> | ||
| h.post( | ||
| path, | ||
| api -> { | ||
| if (retry[0] < 1) { | ||
| api.getResponse() | ||
| .status(429) | ||
| .addHeader("x-ratelimit-reset", "0") | ||
| .send(); | ||
| retry[0]++; | ||
| } else { | ||
| api.getResponse().status(200).send(); | ||
| } | ||
| }))); | ||
| DDIntakeApi client = createIntakeApi(intake.getAddress().toString(), trackType); | ||
| Payload payload = prepareTraces(trackType, Collections.emptyList()); | ||
|
|
||
| try { | ||
| RemoteApi.Response clientResponse = client.sendSerializedTraces(payload); | ||
| assertTrue(clientResponse.success()); | ||
| assertTrue(clientResponse.status().isPresent()); | ||
| assertEquals(200, clientResponse.status().getAsInt()); | ||
| assertEquals(path, intake.getLastRequest().getPath()); | ||
| } finally { | ||
| intake.close(); | ||
| } |
There was a problem hiding this comment.
nitpick: FYI, I'd use the try with resources: #11619 (comment)
But I understand why it's not, so it's a nitpick, rather than something to fix
| try { | ||
| // heartbeat occurs automatically | ||
| long deadline = System.currentTimeMillis() + 5000; | ||
| while (System.currentTimeMillis() < deadline) { | ||
| if (flushCount.get() > 0) break; | ||
| Thread.sleep(50); | ||
| } | ||
| assertTrue(flushCount.get() > 0); | ||
| } finally { | ||
| // cleanup | ||
| worker.close(); | ||
| } |
There was a problem hiding this comment.
note: Note this one is not using try with resources
| ByteBuffer dictionaryBytes = dictionaryBuffer.slice(); | ||
| Map<String, String> meta = new HashMap<>(); | ||
|
|
||
| MessageUnpacker dictionaryUnpacker = MessagePack.newDefaultUnpacker(dictionaryBytes); |
There was a problem hiding this comment.
note: MessageUnpacker is closeable as well, maybe it needs to be within a try with resources
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
Build pipeline has failing jobs for 347f5a4:
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
Build pipeline has failing jobs for fe0bc43: 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
|
What Does This Do
we migrate 17 tests:
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
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 issue/merge. You can also:/merge --commit-message "..."/merge -c/merge -f --reason "reason"; please use this judiciously, as some checks do not run at the PR-level (note: the PR still needs to be mergeable, this will only skip the pre-merge build)Jira ticket: [PROJ-IDENT]