Migrate dd-trace-core groovy files to java part 5#11217
Open
Migrate dd-trace-core groovy files to java part 5#11217
Conversation
we migrate 4 tests: - DDSpanContextTest - DDSpanSerializationTest - DDSpanTest - KnuthSamplingRateTest
Contributor
|
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: b4683a66fe
ℹ️ 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".
|
|
||
| @AfterAll | ||
| static void afterAll() { | ||
| ProcessTags.reset(Config.get()); |
There was a problem hiding this comment.
@WithConfig(...value = "false") disables process-tag propagation for the whole class, but afterAll() calls ProcessTags.reset(Config.get()) before the WithConfigExtension restores system properties, so it reinitializes ProcessTags with the disabled config. Because ProcessTags is global static state, subsequent test classes in the same JVM can observe process tags still disabled and become order-dependent/flaky; the previous Groovy version explicitly set the config back to true before resetting to avoid this leak.
Useful? React with 👍 / 👎.
Contributor
There was a problem hiding this comment.
This looks sound, but previous code was already doing that ; ProcessTags.reset() is actually performing a ProcessTags.reset(Config.get()).
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 62 metrics, 9 unstable metrics. Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.62.0-SNAPSHOT~b4683a66fe0, baseline=1.62.0-SNAPSHOT~75fe2b3c55
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.063 s) : 0, 1062790
Total [baseline] (11.114 s) : 0, 11114392
Agent [candidate] (1.056 s) : 0, 1055822
Total [candidate] (10.984 s) : 0, 10984083
section appsec
Agent [baseline] (1.283 s) : 0, 1282919
Total [baseline] (11.137 s) : 0, 11137107
Agent [candidate] (1.262 s) : 0, 1261505
Total [candidate] (11.043 s) : 0, 11043097
section iast
Agent [baseline] (1.246 s) : 0, 1245570
Total [baseline] (11.349 s) : 0, 11349500
Agent [candidate] (1.236 s) : 0, 1235713
Total [candidate] (11.352 s) : 0, 11352481
section profiling
Agent [baseline] (1.186 s) : 0, 1186088
Total [baseline] (10.957 s) : 0, 10957496
Agent [candidate] (1.195 s) : 0, 1194901
Total [candidate] (11.154 s) : 0, 11153854
gantt
title petclinic - break down per module: candidate=1.62.0-SNAPSHOT~b4683a66fe0, baseline=1.62.0-SNAPSHOT~75fe2b3c55
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.228 ms) : 0, 1228
crashtracking [candidate] (1.23 ms) : 0, 1230
BytebuddyAgent [baseline] (635.283 ms) : 0, 635283
BytebuddyAgent [candidate] (635.406 ms) : 0, 635406
AgentMeter [baseline] (29.517 ms) : 0, 29517
AgentMeter [candidate] (29.384 ms) : 0, 29384
GlobalTracer [baseline] (248.677 ms) : 0, 248677
GlobalTracer [candidate] (248.866 ms) : 0, 248866
AppSec [baseline] (32.763 ms) : 0, 32763
AppSec [candidate] (32.293 ms) : 0, 32293
Debugger [baseline] (60.569 ms) : 0, 60569
Debugger [candidate] (60.192 ms) : 0, 60192
Remote Config [baseline] (602.88 µs) : 0, 603
Remote Config [candidate] (600.218 µs) : 0, 600
Telemetry [baseline] (8.325 ms) : 0, 8325
Telemetry [candidate] (8.075 ms) : 0, 8075
Flare Poller [baseline] (9.713 ms) : 0, 9713
Flare Poller [candidate] (3.567 ms) : 0, 3567
section appsec
crashtracking [baseline] (1.245 ms) : 0, 1245
crashtracking [candidate] (1.215 ms) : 0, 1215
BytebuddyAgent [baseline] (684.993 ms) : 0, 684993
BytebuddyAgent [candidate] (674.976 ms) : 0, 674976
AgentMeter [baseline] (12.389 ms) : 0, 12389
AgentMeter [candidate] (12.275 ms) : 0, 12275
GlobalTracer [baseline] (252.336 ms) : 0, 252336
GlobalTracer [candidate] (248.933 ms) : 0, 248933
AppSec [baseline] (187.037 ms) : 0, 187037
AppSec [candidate] (185.841 ms) : 0, 185841
Debugger [baseline] (65.714 ms) : 0, 65714
Debugger [candidate] (65.766 ms) : 0, 65766
Remote Config [baseline] (580.988 µs) : 0, 581
Remote Config [candidate] (586.182 µs) : 0, 586
Telemetry [baseline] (8.006 ms) : 0, 8006
Telemetry [candidate] (7.84 ms) : 0, 7840
Flare Poller [baseline] (8.623 ms) : 0, 8623
Flare Poller [candidate] (3.4 ms) : 0, 3400
IAST [baseline] (25.008 ms) : 0, 25008
IAST [candidate] (24.29 ms) : 0, 24290
section iast
crashtracking [baseline] (1.217 ms) : 0, 1217
crashtracking [candidate] (1.221 ms) : 0, 1221
BytebuddyAgent [baseline] (822.949 ms) : 0, 822949
BytebuddyAgent [candidate] (811.668 ms) : 0, 811668
AgentMeter [baseline] (11.368 ms) : 0, 11368
AgentMeter [candidate] (11.383 ms) : 0, 11383
GlobalTracer [baseline] (238.685 ms) : 0, 238685
GlobalTracer [candidate] (239.709 ms) : 0, 239709
AppSec [baseline] (31.488 ms) : 0, 31488
AppSec [candidate] (28.841 ms) : 0, 28841
Debugger [baseline] (63.377 ms) : 0, 63377
Debugger [candidate] (65.821 ms) : 0, 65821
Remote Config [baseline] (536.136 µs) : 0, 536
Remote Config [candidate] (560.516 µs) : 0, 561
Telemetry [baseline] (8.037 ms) : 0, 8037
Telemetry [candidate] (7.832 ms) : 0, 7832
Flare Poller [baseline] (3.405 ms) : 0, 3405
Flare Poller [candidate] (3.462 ms) : 0, 3462
IAST [baseline] (28.448 ms) : 0, 28448
IAST [candidate] (29.228 ms) : 0, 29228
section profiling
crashtracking [baseline] (1.179 ms) : 0, 1179
crashtracking [candidate] (1.195 ms) : 0, 1195
BytebuddyAgent [baseline] (692.5 ms) : 0, 692500
BytebuddyAgent [candidate] (697.288 ms) : 0, 697288
AgentMeter [baseline] (8.941 ms) : 0, 8941
AgentMeter [candidate] (9.067 ms) : 0, 9067
GlobalTracer [baseline] (207.658 ms) : 0, 207658
GlobalTracer [candidate] (210.125 ms) : 0, 210125
AppSec [baseline] (32.588 ms) : 0, 32588
AppSec [candidate] (33.045 ms) : 0, 33045
Debugger [baseline] (65.841 ms) : 0, 65841
Debugger [candidate] (66.11 ms) : 0, 66110
Remote Config [baseline] (568.971 µs) : 0, 569
Remote Config [candidate] (583.018 µs) : 0, 583
Telemetry [baseline] (8.067 ms) : 0, 8067
Telemetry [candidate] (7.905 ms) : 0, 7905
Flare Poller [baseline] (3.556 ms) : 0, 3556
Flare Poller [candidate] (3.56 ms) : 0, 3560
ProfilingAgent [baseline] (94.031 ms) : 0, 94031
ProfilingAgent [candidate] (94.302 ms) : 0, 94302
Profiling [baseline] (94.586 ms) : 0, 94586
Profiling [candidate] (94.878 ms) : 0, 94878
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.62.0-SNAPSHOT~b4683a66fe0, baseline=1.62.0-SNAPSHOT~75fe2b3c55
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.067 s) : 0, 1066943
Total [baseline] (8.865 s) : 0, 8865280
Agent [candidate] (1.064 s) : 0, 1064111
Total [candidate] (8.846 s) : 0, 8846435
section iast
Agent [baseline] (1.24 s) : 0, 1239686
Total [baseline] (9.575 s) : 0, 9575170
Agent [candidate] (1.233 s) : 0, 1232640
Total [candidate] (9.528 s) : 0, 9527657
gantt
title insecure-bank - break down per module: candidate=1.62.0-SNAPSHOT~b4683a66fe0, baseline=1.62.0-SNAPSHOT~75fe2b3c55
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.256 ms) : 0, 1256
crashtracking [candidate] (1.238 ms) : 0, 1238
BytebuddyAgent [baseline] (637.646 ms) : 0, 637646
BytebuddyAgent [candidate] (638.618 ms) : 0, 638618
AgentMeter [baseline] (29.444 ms) : 0, 29444
AgentMeter [candidate] (29.444 ms) : 0, 29444
GlobalTracer [baseline] (249.404 ms) : 0, 249404
GlobalTracer [candidate] (250.087 ms) : 0, 250087
AppSec [baseline] (33.02 ms) : 0, 33020
AppSec [candidate] (32.474 ms) : 0, 32474
Debugger [baseline] (60.314 ms) : 0, 60314
Debugger [candidate] (59.732 ms) : 0, 59732
Remote Config [baseline] (613.614 µs) : 0, 614
Remote Config [candidate] (610.55 µs) : 0, 611
Telemetry [baseline] (10.059 ms) : 0, 10059
Telemetry [candidate] (9.712 ms) : 0, 9712
Flare Poller [baseline] (9.174 ms) : 0, 9174
Flare Poller [candidate] (6.003 ms) : 0, 6003
section iast
crashtracking [baseline] (1.221 ms) : 0, 1221
crashtracking [candidate] (1.221 ms) : 0, 1221
BytebuddyAgent [baseline] (820.933 ms) : 0, 820933
BytebuddyAgent [candidate] (811.42 ms) : 0, 811420
AgentMeter [baseline] (11.263 ms) : 0, 11263
AgentMeter [candidate] (11.4 ms) : 0, 11400
GlobalTracer [baseline] (236.918 ms) : 0, 236918
GlobalTracer [candidate] (239.093 ms) : 0, 239093
AppSec [baseline] (31.286 ms) : 0, 31286
AppSec [candidate] (29.315 ms) : 0, 29315
Debugger [baseline] (62.159 ms) : 0, 62159
Debugger [candidate] (62.533 ms) : 0, 62533
Remote Config [baseline] (522.692 µs) : 0, 523
Remote Config [candidate] (525.596 µs) : 0, 526
Telemetry [baseline] (7.939 ms) : 0, 7939
Telemetry [candidate] (7.705 ms) : 0, 7705
Flare Poller [baseline] (3.316 ms) : 0, 3316
Flare Poller [candidate] (3.406 ms) : 0, 3406
IAST [baseline] (28.166 ms) : 0, 28166
IAST [candidate] (30.04 ms) : 0, 30040
LoadParameters
See matching parameters
SummaryFound 1 performance improvements and 3 performance regressions! Performance is the same for 16 metrics, 16 unstable metrics.
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.62.0-SNAPSHOT~b4683a66fe0, baseline=1.62.0-SNAPSHOT~75fe2b3c55
dateFormat X
axisFormat %s
section baseline
no_agent (19.225 ms) : 19032, 19418
. : milestone, 19225,
appsec (18.707 ms) : 18521, 18894
. : milestone, 18707,
code_origins (17.986 ms) : 17809, 18164
. : milestone, 17986,
iast (17.98 ms) : 17804, 18157
. : milestone, 17980,
profiling (18.974 ms) : 18786, 19163
. : milestone, 18974,
tracing (17.877 ms) : 17698, 18055
. : milestone, 17877,
section candidate
no_agent (19.125 ms) : 18931, 19319
. : milestone, 19125,
appsec (18.901 ms) : 18712, 19091
. : milestone, 18901,
code_origins (17.862 ms) : 17689, 18035
. : milestone, 17862,
iast (19.046 ms) : 18850, 19242
. : milestone, 19046,
profiling (18.368 ms) : 18189, 18548
. : milestone, 18368,
tracing (18.969 ms) : 18780, 19159
. : milestone, 18969,
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.62.0-SNAPSHOT~b4683a66fe0, baseline=1.62.0-SNAPSHOT~75fe2b3c55
dateFormat X
axisFormat %s
section baseline
no_agent (1.236 ms) : 1224, 1248
. : milestone, 1236,
iast (3.311 ms) : 3265, 3357
. : milestone, 3311,
iast_FULL (6.19 ms) : 6125, 6255
. : milestone, 6190,
iast_GLOBAL (3.626 ms) : 3566, 3687
. : milestone, 3626,
profiling (2.23 ms) : 2209, 2251
. : milestone, 2230,
tracing (1.864 ms) : 1848, 1880
. : milestone, 1864,
section candidate
no_agent (1.244 ms) : 1232, 1257
. : milestone, 1244,
iast (3.268 ms) : 3226, 3309
. : milestone, 3268,
iast_FULL (5.944 ms) : 5884, 6005
. : milestone, 5944,
iast_GLOBAL (3.599 ms) : 3543, 3655
. : milestone, 3599,
profiling (2.372 ms) : 2348, 2396
. : milestone, 2372,
tracing (1.857 ms) : 1840, 1874
. : milestone, 1857,
DacapoParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 10 metrics, 2 unstable metrics. Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.62.0-SNAPSHOT~b4683a66fe0, baseline=1.62.0-SNAPSHOT~75fe2b3c55
dateFormat X
axisFormat %s
section baseline
no_agent (15.021 s) : 15021000, 15021000
. : milestone, 15021000,
appsec (14.969 s) : 14969000, 14969000
. : milestone, 14969000,
iast (18.974 s) : 18974000, 18974000
. : milestone, 18974000,
iast_GLOBAL (18.043 s) : 18043000, 18043000
. : milestone, 18043000,
profiling (15.098 s) : 15098000, 15098000
. : milestone, 15098000,
tracing (14.652 s) : 14652000, 14652000
. : milestone, 14652000,
section candidate
no_agent (15.569 s) : 15569000, 15569000
. : milestone, 15569000,
appsec (14.668 s) : 14668000, 14668000
. : milestone, 14668000,
iast (18.789 s) : 18789000, 18789000
. : milestone, 18789000,
iast_GLOBAL (17.9 s) : 17900000, 17900000
. : milestone, 17900000,
profiling (14.797 s) : 14797000, 14797000
. : milestone, 14797000,
tracing (15.035 s) : 15035000, 15035000
. : milestone, 15035000,
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.62.0-SNAPSHOT~b4683a66fe0, baseline=1.62.0-SNAPSHOT~75fe2b3c55
dateFormat X
axisFormat %s
section baseline
no_agent (1.495 ms) : 1484, 1507
. : milestone, 1495,
appsec (3.78 ms) : 3560, 4001
. : milestone, 3780,
iast (2.282 ms) : 2212, 2352
. : milestone, 2282,
iast_GLOBAL (2.326 ms) : 2257, 2396
. : milestone, 2326,
profiling (2.126 ms) : 2070, 2181
. : milestone, 2126,
tracing (2.081 ms) : 2028, 2135
. : milestone, 2081,
section candidate
no_agent (1.49 ms) : 1479, 1502
. : milestone, 1490,
appsec (3.759 ms) : 3542, 3976
. : milestone, 3759,
iast (2.29 ms) : 2220, 2360
. : milestone, 2290,
iast_GLOBAL (2.336 ms) : 2266, 2405
. : milestone, 2336,
profiling (2.536 ms) : 2371, 2702
. : milestone, 2536,
tracing (2.084 ms) : 2031, 2137
. : milestone, 2084,
|
jpbempel
added
comp: testing
Contributor
PerfectSlayer
left a comment
PerfectSlayer
left a comment
There was a problem hiding this comment.
Early review of a single file (DDSpanContextTest). More eyes welcome on this review :)
Comment on lines
+224
to
+239
| case "Integer.MAX_VALUE": | ||
| return Integer.MAX_VALUE; | ||
| case "Integer.MIN_VALUE": | ||
| return Integer.MIN_VALUE; | ||
| case "Short.MAX_VALUE": | ||
| return Short.MAX_VALUE; | ||
| case "Short.MIN_VALUE": | ||
| return Short.MIN_VALUE; | ||
| case "Float.MAX_VALUE": | ||
| return Float.MAX_VALUE; | ||
| case "Float.MIN_VALUE": | ||
| return Float.MIN_VALUE; | ||
| case "Double.MAX_VALUE": | ||
| return Double.MAX_VALUE; | ||
| case "Double.MIN_VALUE": | ||
| return Double.MIN_VALUE; |
Contributor
There was a problem hiding this comment.
❔ question: Should this be part of the common type converter?
| "scenario | rate | limit ", | ||
| "rate=1.0 limit=10 | 1.0 | 10 ", | ||
| "rate=0.5 limit=100 | 0.5 | 100 ", | ||
| "rate=0.25 no limit | 0.25 | 2147483647" |
Contributor
There was a problem hiding this comment.
🔨 issue: Set limit to Integer.MAX_VALUE
| } | ||
|
|
||
| @TableTest({ | ||
| "scenario | rate | limit ", |
Contributor
There was a problem hiding this comment.
❔ question: Is scenario useful if it only duplicate parameter values?
Better keep the default behavior without scenario to avoid duplication.
| assertEquals(DDSpanContext.SPAN_KIND_CUSTOM + 1, DDSpanContext.SPAN_KIND_VALUES.length); | ||
|
|
||
| assertEquals( | ||
| Tags.SPAN_KIND_SERVER, DDSpanContext.SPAN_KIND_VALUES[DDSpanContext.SPAN_KIND_SERVER]); |
Contributor
There was a problem hiding this comment.
🎯 suggestion: What about using static imports for Tags, DDTags, and DDSpanContext to improve readability?
Member
Author
There was a problem hiding this comment.
there are ambiguity here (Tags.SPAN_KIND_SERVER vs DDSpanContext.SPAN_KIND_SERVER)
but tried to do the rest
Comment on lines
+462
to
+468
| "scenario | kindString | expectedOrdinal", | ||
| "server | server | 1 ", | ||
| "client | client | 2 ", | ||
| "producer | producer | 3 ", | ||
| "consumer | consumer | 4 ", | ||
| "internal | internal | 5 ", | ||
| "broker | broker | 6 " |
Contributor
There was a problem hiding this comment.
🔨 issue: Ordinal was resolved. It was supposed to be constant references.
| } | ||
|
|
||
| @TableTest({ | ||
| "scenario | kindString", |
Contributor
There was a problem hiding this comment.
🎯 suggestion: Drop duplicate scenario
| } | ||
|
|
||
| @TableTest({ | ||
| "scenario | kindString", |
Contributor
There was a problem hiding this comment.
🎯 suggestion: Drop duplicate scenario
bric3
reviewed
May 6, 2026
| @BeforeEach | ||
| void setup() { | ||
| writer = new ListWriter(); | ||
| profilingContextIntegration = mock(ProfilingContextIntegration.class); |
Contributor
There was a problem hiding this comment.
nitpick: I think it's even possible to write it as following given the field type is already known by the compiler:
Suggested change
| profilingContextIntegration = mock(ProfilingContextIntegration.class); | |
| profilingContextIntegration = mock(); |
Contributor
PerfectSlayer
left a comment
PerfectSlayer
left a comment
There was a problem hiding this comment.
Thanks for the migration. I keep reviewing it and I'm adding review for DDSpanContextTest
Comment on lines
+77
to
+83
| @TableTest({ | ||
| "scenario | name", | ||
| "SERVICE_NAME | " + DDTags.SERVICE_NAME, | ||
| "RESOURCE_NAME | " + DDTags.RESOURCE_NAME, | ||
| "SPAN_TYPE | " + DDTags.SPAN_TYPE, | ||
| "spme.tag | some.tag" | ||
| }) |
Contributor
There was a problem hiding this comment.
🎯 suggestion: I would use @ValueSource only: https://github.com/DataDog/dd-trace-java/blob/master/docs/how_to_test_with_junit.md#valuesource as scenario is a duplicate (and even introduce a typo)
Comment on lines
+101
to
+108
| Thread thread = Thread.currentThread(); | ||
| Map<String, Object> expectedTags = new HashMap<>(); | ||
| if (!name.equals("some.tag")) { | ||
| expectedTags.put("some.tag", "asdf"); | ||
| } | ||
| expectedTags.put(THREAD_NAME, thread.getName()); | ||
| expectedTags.put(THREAD_ID, thread.getId()); | ||
| expectedTags.put(DDTags.DD_SVC_SRC, ServiceNameSources.MANUAL); |
Contributor
There was a problem hiding this comment.
🎯 suggestion: expectedTags with thread and service tags is duplicated multiple times. I would create a static method to initialize it for all the related tests.
It's a bit out of scope of the migration though... 🤔
| default: | ||
| throw new IllegalArgumentException("Unknown method: " + method); | ||
| } | ||
| assertEquals(value, actualValue); |
Contributor
There was a problem hiding this comment.
🎯 suggestion: I would rename the value method parameter as expected and actualValue as value for clarity.
|
|
||
| context.setMetric("test", value); | ||
|
|
||
| assertTrue(expectedType.isInstance(context.getTag("test"))); |
Contributor
There was a problem hiding this comment.
🎯 suggestion: What about using assertInstanceOf()?
Comment on lines
-237
to
-240
| // single span sampling should not change the trace sampling priority | ||
| context.getSamplingPriority() == UNSET | ||
| // make sure the `_dd.p.dm` tag has not been set by single span sampling | ||
| !context.getPropagationTags().createTagMap().containsKey("_dd.p.dm") |
Contributor
There was a problem hiding this comment.
💭 thought: It would be nice if you can keep the comments that explain the test behavior / expectation 🙏
Comment on lines
-313
to
-314
| // even though span ID and parent ID are setup as negative numbers, they should be printed as their unsigned value | ||
| // asserting there is no negative sign after ids is the best I can do. |
Contributor
There was a problem hiding this comment.
💭 thought: Same here about keeping comments
| } | ||
|
|
||
| def "span kind ordinal constants and SPAN_KIND_VALUES array stay in sync"() { | ||
| expect: "SPAN_KIND_VALUES array covers all ordinals" |
Contributor
There was a problem hiding this comment.
💭 thought: We are loosing the comments from expect, and, when that help with test case understanding
bric3
reviewed
May 6, 2026
Contributor
bric3
left a comment
bric3
left a comment
There was a problem hiding this comment.
I noticed that the conversion dropped quite a few inline comments, or named spock clauses that I would find useful when working on the tests.
Maybe the skill can be updated to keep inline comments, and expose as comments the named spock clauses.
| expectedTags.put(DDTags.DD_SVC_SRC, ServiceNameSources.MANUAL); | ||
| assertTagmap(context.getTags(), expectedTags); | ||
|
|
||
| Object actualValue; |
Contributor
There was a problem hiding this comment.
nitpick: This could be a String rather than an Object.
Comment on lines
+163
to
+169
| static Stream<Arguments> tagsCanBeAddedToContextArguments() { | ||
| return Stream.of( | ||
| arguments("tag.name", "some value"), | ||
| arguments("tag with int", 1234), | ||
| arguments("tag-with-bool", false), | ||
| arguments("tag_with_float", 0.321)); | ||
| } |
Contributor
There was a problem hiding this comment.
note: This is purely informational comment, but I think this can be expressed as a pure array.
Suggested change
| static Stream<Arguments> tagsCanBeAddedToContextArguments() { | |
| return Stream.of( | |
| arguments("tag.name", "some value"), | |
| arguments("tag with int", 1234), | |
| arguments("tag-with-bool", false), | |
| arguments("tag_with_float", 0.321)); | |
| } | |
| static Object[][] tagsCanBeAddedToContextArguments() { | |
| return new Object [][] { | |
| { "tag.name", "some value" }, | |
| { "tag with int", 1234 }, | |
| { "tag-with-bool", false }, | |
| { "tag_with_float", 0.321 }}; | |
| } |
| } | ||
|
|
||
| def "metrics use the expected types"() { | ||
| // floats should be converted to doubles. |
Contributor
There was a problem hiding this comment.
question: Is it ? Also I don't see such comments reported to the ported code.
Comment on lines
+325
to
+326
| assertEquals(UNSET, context.getSamplingPriority()); | ||
| assertFalse(context.getPropagationTags().createTagMap().containsKey("_dd.p.dm")); |
Contributor
There was a problem hiding this comment.
suggestion: Comments from the original file
Suggested change
| assertEquals(UNSET, context.getSamplingPriority()); | |
| assertFalse(context.getPropagationTags().createTagMap().containsKey("_dd.p.dm")); | |
| // single span sampling should not change the trace sampling priority | |
| assertEquals(UNSET, context.getSamplingPriority()); | |
| // make sure the `_dd.p.dm` tag has not been set by single span sampling | |
| assertFalse(context.getPropagationTags().createTagMap().containsKey("_dd.p.dm")); |
|
|
||
| DDSpanContext context = (DDSpanContext) span.context(); | ||
|
|
||
| assertFalse(context.toString().contains("id=-")); |
Contributor
There was a problem hiding this comment.
suggestion: original comments
Suggested change
| assertFalse(context.toString().contains("id=-")); | |
| // even though span ID and parent ID are setup as negative numbers, they should be printed as their unsigned value | |
| // asserting there is no negative sign after ids is the best I can do. | |
| assertFalse(context.toString().contains("id=-")); |
Comment on lines
+25
to
+52
| @TableTest({ | ||
| "scenario | rate | expected", | ||
| "1.0 | 1.0 | 1 ", | ||
| "0.5 | 0.5 | 0.5 ", | ||
| "0.1 | 0.1 | 0.1 ", | ||
| "0.0 | 0.0 | 0 ", | ||
| "0.765432 | 0.765432 | 0.765432", | ||
| "0.7654321 rounds to 6 decimal | 0.7654321 | 0.765432", | ||
| "0.123456 | 0.123456 | 0.123456", | ||
| "0.100000 trailing zeros | 0.100000 | 0.1 ", | ||
| "0.250 trailing zero | 0.250 | 0.25 ", | ||
| "0.05 | 0.05 | 0.05 ", | ||
| "0.0123456789 rounds at 6dp | 0.0123456789 | 0.012346", | ||
| "0.001 | 0.001 | 0.001 ", | ||
| "0.00500 trailing zeros | 0.00500 | 0.005 ", | ||
| "0.00123456789 rounds at 6dp | 0.00123456789 | 0.001235", | ||
| "0.0001 | 0.0001 | 0.0001 ", | ||
| "0.000500 trailing zeros | 0.000500 | 0.0005 ", | ||
| "0.000123456789 rounds at 6dp | 0.000123456789 | 0.000123", | ||
| "0.9999995 rounds up to 1 | 0.9999995 | 1 ", | ||
| "0.00001 | 0.00001 | 0.00001 ", | ||
| "0.000050 trailing zeros | 0.000050 | 0.00005 ", | ||
| "1.23456789e-5 rounds at 6dp | 0.0000123456789 | 0.000012", | ||
| "1e-7 below precision rounds to 0 | 0.0000001 | 0 ", | ||
| "5.5e-10 below precision | 0.00000000055 | 0 ", | ||
| "0.000001 six decimal boundary | 0.000001 | 0.000001", | ||
| "0.00000051 rounds up | 0.00000051 | 0.000001" | ||
| }) |
Contributor
There was a problem hiding this comment.
suggestion: 6dp -> 6 decimal places
Suggested change
| @TableTest({ | |
| "scenario | rate | expected", | |
| "1.0 | 1.0 | 1 ", | |
| "0.5 | 0.5 | 0.5 ", | |
| "0.1 | 0.1 | 0.1 ", | |
| "0.0 | 0.0 | 0 ", | |
| "0.765432 | 0.765432 | 0.765432", | |
| "0.7654321 rounds to 6 decimal | 0.7654321 | 0.765432", | |
| "0.123456 | 0.123456 | 0.123456", | |
| "0.100000 trailing zeros | 0.100000 | 0.1 ", | |
| "0.250 trailing zero | 0.250 | 0.25 ", | |
| "0.05 | 0.05 | 0.05 ", | |
| "0.0123456789 rounds at 6dp | 0.0123456789 | 0.012346", | |
| "0.001 | 0.001 | 0.001 ", | |
| "0.00500 trailing zeros | 0.00500 | 0.005 ", | |
| "0.00123456789 rounds at 6dp | 0.00123456789 | 0.001235", | |
| "0.0001 | 0.0001 | 0.0001 ", | |
| "0.000500 trailing zeros | 0.000500 | 0.0005 ", | |
| "0.000123456789 rounds at 6dp | 0.000123456789 | 0.000123", | |
| "0.9999995 rounds up to 1 | 0.9999995 | 1 ", | |
| "0.00001 | 0.00001 | 0.00001 ", | |
| "0.000050 trailing zeros | 0.000050 | 0.00005 ", | |
| "1.23456789e-5 rounds at 6dp | 0.0000123456789 | 0.000012", | |
| "1e-7 below precision rounds to 0 | 0.0000001 | 0 ", | |
| "5.5e-10 below precision | 0.00000000055 | 0 ", | |
| "0.000001 six decimal boundary | 0.000001 | 0.000001", | |
| "0.00000051 rounds up | 0.00000051 | 0.000001" | |
| }) | |
| @TableTest({ | |
| "scenario | rate | expected", | |
| "1.0 | 1.0 | 1 ", | |
| "0.5 | 0.5 | 0.5 ", | |
| "0.1 | 0.1 | 0.1 ", | |
| "0.0 | 0.0 | 0 ", | |
| "0.765432 | 0.765432 | 0.765432", | |
| "0.7654321 rounds to 6 decimal | 0.7654321 | 0.765432", | |
| "0.123456 | 0.123456 | 0.123456", | |
| "0.100000 trailing zeros | 0.100000 | 0.1 ", | |
| "0.250 trailing zero | 0.250 | 0.25 ", | |
| "0.05 | 0.05 | 0.05 ", | |
| "0.0123456789 rounds at 6 decimal places | 0.0123456789 | 0.012346", | |
| "0.001 | 0.001 | 0.001 ", | |
| "0.00500 trailing zeros | 0.00500 | 0.005 ", | |
| "0.00123456789 rounds at 6 decimal places | 0.00123456789 | 0.001235", | |
| "0.0001 | 0.0001 | 0.0001 ", | |
| "0.000500 trailing zeros | 0.000500 | 0.0005 ", | |
| "0.000123456789 rounds at 6 decimal places | 0.000123456789 | 0.000123", | |
| "0.9999995 rounds up to 1 | 0.9999995 | 1 ", | |
| "0.00001 | 0.00001 | 0.00001 ", | |
| "0.000050 trailing zeros | 0.000050 | 0.00005 ", | |
| "1.23456789e-5 rounds at 6 decimal places | 0.0000123456789 | 0.000012", | |
| "1e-7 below precision rounds to 0 | 0.0000001 | 0 ", | |
| "5.5e-10 below precision | 0.00000000055 | 0 ", | |
| "0.000001 six decimal boundary | 0.000001 | 0.000001", | |
| "0.00000051 rounds up | 0.00000051 | 0.000001" | |
| }) |
Although I wonder is other comment in the original tests are useful to understand the ksr.
Comment on lines
+93
to
+98
| @TableTest({ | ||
| "scenario | jsonRules | expectedKsr", | ||
| "rate 1 matches | '[{\"service\": \"service\", \"sample_rate\": 1}]' | 1 ", | ||
| "rate 0.5 matches | '[{\"service\": \"service\", \"sample_rate\": 0.5}]' | 0.5 ", | ||
| "rate 0 matches | '[{\"service\": \"service\", \"sample_rate\": 0}]' | 0 " | ||
| }) |
Contributor
There was a problem hiding this comment.
suggestion: Closer scenario names to what was in groovy file
Suggested change
| @TableTest({ | |
| "scenario | jsonRules | expectedKsr", | |
| "rate 1 matches | '[{\"service\": \"service\", \"sample_rate\": 1}]' | 1 ", | |
| "rate 0.5 matches | '[{\"service\": \"service\", \"sample_rate\": 0.5}]' | 0.5 ", | |
| "rate 0 matches | '[{\"service\": \"service\", \"sample_rate\": 0}]' | 0 " | |
| }) | |
| @TableTest({ | |
| "scenario | jsonRules | expectedKsr", | |
| "Matching rule with rate 1 -> ksr is 1 | '[{\"service\": \"service\", \"sample_rate\": 1}]' | 1 ", | |
| "Matching rule with rate 0.5 -> ksr is 0.5 | '[{\"service\": \"service\", \"sample_rate\": 0.5}]' | 0.5 ", | |
| "Matching rule with rate 0 -> ksr is 0 (drop, but ksr still set) | '[{\"service\": \"service\", \"sample_rate\": 0}]' | 0 " | |
| }) |
| @Test | ||
| void ruleBasedSamplerFallbackToAgentSamplerSetsKsr() { | ||
| Properties properties = new Properties(); | ||
| properties.setProperty( |
Contributor
There was a problem hiding this comment.
suggestion:
Suggested change
| properties.setProperty( | |
| // Rule that does NOT match "service" | |
| properties.setProperty( |
| Map<String, String> propagationMap = span.context().getPropagationTags().createTagMap(); | ||
| String ksr = propagationMap.get("_dd.p.ksr"); | ||
|
|
||
| assertEquals("1", ksr); |
Contributor
There was a problem hiding this comment.
suggestion:
Suggested change
| assertEquals("1", ksr); | |
| // When falling back to agent sampler, ksr should still be set (agent rate = 1.0 by default) | |
| assertEquals("1", ksr); |
| import org.tabletest.junit.TypeConverter; | ||
|
|
||
| /** Shared converters for JUnit 5 TableTest tests that use unparsable constants. */ | ||
| public final class TableTestTypeConverters { |
Contributor
There was a problem hiding this comment.
suggestion: What about making clear this converter is for Java types ?
Suggested change
| public final class TableTestJavaTypeConverters { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Does This Do
we migrate 4 tests:
Motivation
this is part of the effort to migrate groovy tests to Java/JUnit
part1: #11053
part2: #11062
part3: #11085
part4: #11146
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]
Note: Once your PR is ready to merge, add it to the merge queue by commenting
/merge./merge -ccancels the queue request./merge -f --reason "reason"skips all merge queue checks; please use this judiciously, as some checks do not run at the PR-level. For more information, see this doc.