Migrate dd-trace-core groovy files to java part 2#11062
Open
Migrate dd-trace-core groovy files to java part 2#11062
Conversation
jpbempel
force-pushed
the
jpbempel/g2j-DDSpec
branch
2 times, most recently
from
35225f3 to
7e1ec4c
Compare
Contributor
PerfectSlayer
left a comment
PerfectSlayer
left a comment
There was a problem hiding this comment.
Dropping few comments from a partial early review
Comment on lines
+53
to
+57
| static { | ||
| allowContextTesting(); | ||
| installConfigTransformer(); | ||
| makeConfigInstanceModifiable(); | ||
| } |
Contributor
There was a problem hiding this comment.
❔ question: Can this go to a @BeforeAll method?
Comment on lines
+43
to
+44
| private ListWriter writer = new ListWriter(); | ||
| private CoreTracer tracer = tracerBuilder().writer(writer).build(); |
Contributor
There was a problem hiding this comment.
🎯 suggestion: Moving initialization into a @BeforeEach method?
|
|
||
| private static final int SPAN_LINK_TAG_MAX_LENGTH = 25_000; | ||
| private static final ObjectMapper JSON_MAPPER = new ObjectMapper(); | ||
| // W3C Trace Context standard header names (W3CHttpCodec is package-private) |
Contributor
There was a problem hiding this comment.
❔ question: Should we add an test helper to access them rather than duplicating?
Member
Author
There was a problem hiding this comment.
yep I have already a bridge for accessing the method factory
| "sampled | true | '01' | '1' ", | ||
| "not sampled | false | '00' | '-1' " | ||
| }) | ||
| @ParameterizedTest(name = "create span link from extracted context [{index}]") |
Contributor
There was a problem hiding this comment.
🎯 suggestion: Better use scenario than index for readability.
EDIT: That should be the default behavior if we drop the @ParameterizedTest as it does not add value compared to the test method name.
Suggested change
| @ParameterizedTest(name = "create span link from extracted context [{index}]") |
Comment on lines
+95
to
+99
| JSON_MAPPER.readValue( | ||
| spanLinksTag, | ||
| JSON_MAPPER | ||
| .getTypeFactory() | ||
| .constructCollectionType(List.class, TestSpanLinkJson.class)); |
Contributor
There was a problem hiding this comment.
🎯 suggestion: This feels it should go to the TestSpanlinkJson class. It's duplicated among test cases too.
Member
Author
There was a problem hiding this comment.
added for now just a static method to deserialize
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 58 metrics, 13 unstable metrics. Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.61.0-SNAPSHOT~9e0bebb6dd, baseline=1.61.0-SNAPSHOT~aa7c70f2e7
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.058 s) : 0, 1058156
Total [baseline] (8.836 s) : 0, 8836468
Agent [candidate] (1.064 s) : 0, 1064493
Total [candidate] (8.851 s) : 0, 8850688
section iast
Agent [baseline] (1.24 s) : 0, 1240456
Total [baseline] (9.583 s) : 0, 9582630
Agent [candidate] (1.234 s) : 0, 1234165
Total [candidate] (9.6 s) : 0, 9599659
gantt
title insecure-bank - break down per module: candidate=1.61.0-SNAPSHOT~9e0bebb6dd, baseline=1.61.0-SNAPSHOT~aa7c70f2e7
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.236 ms) : 0, 1236
crashtracking [candidate] (1.249 ms) : 0, 1249
BytebuddyAgent [baseline] (633.215 ms) : 0, 633215
BytebuddyAgent [candidate] (638.307 ms) : 0, 638307
AgentMeter [baseline] (29.614 ms) : 0, 29614
AgentMeter [candidate] (29.682 ms) : 0, 29682
GlobalTracer [baseline] (249.696 ms) : 0, 249696
GlobalTracer [candidate] (250.204 ms) : 0, 250204
AppSec [baseline] (32.269 ms) : 0, 32269
AppSec [candidate] (32.114 ms) : 0, 32114
Debugger [baseline] (59.681 ms) : 0, 59681
Debugger [candidate] (59.5 ms) : 0, 59500
Remote Config [baseline] (609.192 µs) : 0, 609
Remote Config [candidate] (596.196 µs) : 0, 596
Telemetry [baseline] (8.179 ms) : 0, 8179
Telemetry [candidate] (8.188 ms) : 0, 8188
Flare Poller [baseline] (7.34 ms) : 0, 7340
Flare Poller [candidate] (8.249 ms) : 0, 8249
section iast
crashtracking [baseline] (1.261 ms) : 0, 1261
crashtracking [candidate] (1.242 ms) : 0, 1242
BytebuddyAgent [baseline] (812.932 ms) : 0, 812932
BytebuddyAgent [candidate] (808.821 ms) : 0, 808821
AgentMeter [baseline] (11.777 ms) : 0, 11777
AgentMeter [candidate] (11.583 ms) : 0, 11583
GlobalTracer [baseline] (242.042 ms) : 0, 242042
GlobalTracer [candidate] (241.314 ms) : 0, 241314
AppSec [baseline] (33.143 ms) : 0, 33143
AppSec [candidate] (30.455 ms) : 0, 30455
Debugger [baseline] (58.681 ms) : 0, 58681
Debugger [candidate] (62.144 ms) : 0, 62144
Remote Config [baseline] (540.913 µs) : 0, 541
Remote Config [candidate] (528.439 µs) : 0, 528
Telemetry [baseline] (13.118 ms) : 0, 13118
Telemetry [candidate] (12.284 ms) : 0, 12284
Flare Poller [baseline] (3.538 ms) : 0, 3538
Flare Poller [candidate] (3.52 ms) : 0, 3520
IAST [baseline] (26.956 ms) : 0, 26956
IAST [candidate] (25.856 ms) : 0, 25856
Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.61.0-SNAPSHOT~9e0bebb6dd, baseline=1.61.0-SNAPSHOT~aa7c70f2e7
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.057 s) : 0, 1056784
Total [baseline] (11.022 s) : 0, 11021643
Agent [candidate] (1.054 s) : 0, 1054154
Total [candidate] (11.151 s) : 0, 11151003
section appsec
Agent [baseline] (1.256 s) : 0, 1255816
Total [baseline] (11.084 s) : 0, 11083549
Agent [candidate] (1.249 s) : 0, 1248502
Total [candidate] (11.139 s) : 0, 11138689
section iast
Agent [baseline] (1.224 s) : 0, 1224437
Total [baseline] (11.306 s) : 0, 11305910
Agent [candidate] (1.221 s) : 0, 1221336
Total [candidate] (11.291 s) : 0, 11290881
section profiling
Agent [baseline] (1.184 s) : 0, 1183743
Total [baseline] (11.103 s) : 0, 11102734
Agent [candidate] (1.189 s) : 0, 1188751
Total [candidate] (11.04 s) : 0, 11039725
gantt
title petclinic - break down per module: candidate=1.61.0-SNAPSHOT~9e0bebb6dd, baseline=1.61.0-SNAPSHOT~aa7c70f2e7
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.234 ms) : 0, 1234
crashtracking [candidate] (1.218 ms) : 0, 1218
BytebuddyAgent [baseline] (633.922 ms) : 0, 633922
BytebuddyAgent [candidate] (631.942 ms) : 0, 631942
AgentMeter [baseline] (29.475 ms) : 0, 29475
AgentMeter [candidate] (29.214 ms) : 0, 29214
GlobalTracer [baseline] (249.791 ms) : 0, 249791
GlobalTracer [candidate] (248.485 ms) : 0, 248485
AppSec [baseline] (32.145 ms) : 0, 32145
AppSec [candidate] (31.968 ms) : 0, 31968
Debugger [baseline] (60.153 ms) : 0, 60153
Debugger [candidate] (59.937 ms) : 0, 59937
Remote Config [baseline] (603.815 µs) : 0, 604
Remote Config [candidate] (588.705 µs) : 0, 589
Telemetry [baseline] (8.068 ms) : 0, 8068
Telemetry [candidate] (8.047 ms) : 0, 8047
Flare Poller [baseline] (5.156 ms) : 0, 5156
Flare Poller [candidate] (6.631 ms) : 0, 6631
section appsec
crashtracking [baseline] (1.241 ms) : 0, 1241
crashtracking [candidate] (1.236 ms) : 0, 1236
BytebuddyAgent [baseline] (666.274 ms) : 0, 666274
BytebuddyAgent [candidate] (661.06 ms) : 0, 661060
AgentMeter [baseline] (12.043 ms) : 0, 12043
AgentMeter [candidate] (12.069 ms) : 0, 12069
GlobalTracer [baseline] (250.198 ms) : 0, 250198
GlobalTracer [candidate] (249.387 ms) : 0, 249387
IAST [baseline] (24.645 ms) : 0, 24645
IAST [candidate] (24.604 ms) : 0, 24604
AppSec [baseline] (185.406 ms) : 0, 185406
AppSec [candidate] (184.837 ms) : 0, 184837
Debugger [baseline] (66.719 ms) : 0, 66719
Debugger [candidate] (66.143 ms) : 0, 66143
Remote Config [baseline] (601.805 µs) : 0, 602
Remote Config [candidate] (624.526 µs) : 0, 625
Telemetry [baseline] (8.603 ms) : 0, 8603
Telemetry [candidate] (8.558 ms) : 0, 8558
Flare Poller [baseline] (3.584 ms) : 0, 3584
Flare Poller [candidate] (3.559 ms) : 0, 3559
section iast
crashtracking [baseline] (1.233 ms) : 0, 1233
crashtracking [candidate] (1.211 ms) : 0, 1211
BytebuddyAgent [baseline] (801.235 ms) : 0, 801235
BytebuddyAgent [candidate] (798.83 ms) : 0, 798830
AgentMeter [baseline] (11.353 ms) : 0, 11353
AgentMeter [candidate] (11.385 ms) : 0, 11385
GlobalTracer [baseline] (239.176 ms) : 0, 239176
GlobalTracer [candidate] (238.617 ms) : 0, 238617
IAST [baseline] (25.808 ms) : 0, 25808
IAST [candidate] (25.683 ms) : 0, 25683
AppSec [baseline] (29.434 ms) : 0, 29434
AppSec [candidate] (34.431 ms) : 0, 34431
Debugger [baseline] (63.543 ms) : 0, 63543
Debugger [candidate] (58.006 ms) : 0, 58006
Remote Config [baseline] (2.343 ms) : 0, 2343
Remote Config [candidate] (1.158 ms) : 0, 1158
Telemetry [baseline] (10.669 ms) : 0, 10669
Telemetry [candidate] (12.446 ms) : 0, 12446
Flare Poller [baseline] (3.452 ms) : 0, 3452
Flare Poller [candidate] (3.425 ms) : 0, 3425
section profiling
crashtracking [baseline] (1.177 ms) : 0, 1177
crashtracking [candidate] (1.177 ms) : 0, 1177
BytebuddyAgent [baseline] (690.831 ms) : 0, 690831
BytebuddyAgent [candidate] (693.19 ms) : 0, 693190
AgentMeter [baseline] (9.212 ms) : 0, 9212
AgentMeter [candidate] (9.161 ms) : 0, 9161
GlobalTracer [baseline] (206.827 ms) : 0, 206827
GlobalTracer [candidate] (208.632 ms) : 0, 208632
AppSec [baseline] (32.473 ms) : 0, 32473
AppSec [candidate] (32.816 ms) : 0, 32816
Debugger [baseline] (65.616 ms) : 0, 65616
Debugger [candidate] (66.331 ms) : 0, 66331
Remote Config [baseline] (570.754 µs) : 0, 571
Remote Config [candidate] (580.35 µs) : 0, 580
Telemetry [baseline] (7.856 ms) : 0, 7856
Telemetry [candidate] (7.795 ms) : 0, 7795
Flare Poller [baseline] (3.569 ms) : 0, 3569
Flare Poller [candidate] (3.549 ms) : 0, 3549
ProfilingAgent [baseline] (94.211 ms) : 0, 94211
ProfilingAgent [candidate] (94.112 ms) : 0, 94112
Profiling [baseline] (94.786 ms) : 0, 94786
Profiling [candidate] (94.686 ms) : 0, 94686
LoadParameters
See matching parameters
SummaryFound 2 performance improvements and 3 performance regressions! Performance is the same for 14 metrics, 17 unstable metrics.
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.61.0-SNAPSHOT~9e0bebb6dd, baseline=1.61.0-SNAPSHOT~aa7c70f2e7
dateFormat X
axisFormat %s
section baseline
no_agent (1.243 ms) : 1231, 1255
. : milestone, 1243,
iast (3.315 ms) : 3266, 3364
. : milestone, 3315,
iast_FULL (5.886 ms) : 5827, 5946
. : milestone, 5886,
iast_GLOBAL (3.758 ms) : 3695, 3822
. : milestone, 3758,
profiling (2.184 ms) : 2165, 2203
. : milestone, 2184,
tracing (1.91 ms) : 1892, 1928
. : milestone, 1910,
section candidate
no_agent (1.25 ms) : 1237, 1262
. : milestone, 1250,
iast (3.379 ms) : 3333, 3425
. : milestone, 3379,
iast_FULL (6.167 ms) : 6104, 6231
. : milestone, 6167,
iast_GLOBAL (3.7 ms) : 3639, 3762
. : milestone, 3700,
profiling (2.362 ms) : 2339, 2385
. : milestone, 2362,
tracing (1.943 ms) : 1926, 1961
. : milestone, 1943,
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.61.0-SNAPSHOT~9e0bebb6dd, baseline=1.61.0-SNAPSHOT~aa7c70f2e7
dateFormat X
axisFormat %s
section baseline
no_agent (18.271 ms) : 18083, 18460
. : milestone, 18271,
appsec (18.664 ms) : 18478, 18851
. : milestone, 18664,
code_origins (18.091 ms) : 17911, 18272
. : milestone, 18091,
iast (19.275 ms) : 19079, 19471
. : milestone, 19275,
profiling (18.604 ms) : 18416, 18792
. : milestone, 18604,
tracing (17.883 ms) : 17704, 18061
. : milestone, 17883,
section candidate
no_agent (19.779 ms) : 19579, 19978
. : milestone, 19779,
appsec (20.171 ms) : 19961, 20381
. : milestone, 20171,
code_origins (17.853 ms) : 17678, 18029
. : milestone, 17853,
iast (17.888 ms) : 17710, 18066
. : milestone, 17888,
profiling (18.618 ms) : 18433, 18803
. : milestone, 18618,
tracing (17.823 ms) : 17648, 17999
. : milestone, 17823,
DacapoParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 11 metrics, 1 unstable metrics. Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.61.0-SNAPSHOT~9e0bebb6dd, baseline=1.61.0-SNAPSHOT~aa7c70f2e7
dateFormat X
axisFormat %s
section baseline
no_agent (14.747 s) : 14747000, 14747000
. : milestone, 14747000,
appsec (15.041 s) : 15041000, 15041000
. : milestone, 15041000,
iast (18.47 s) : 18470000, 18470000
. : milestone, 18470000,
iast_GLOBAL (17.992 s) : 17992000, 17992000
. : milestone, 17992000,
profiling (14.998 s) : 14998000, 14998000
. : milestone, 14998000,
tracing (15.018 s) : 15018000, 15018000
. : milestone, 15018000,
section candidate
no_agent (14.847 s) : 14847000, 14847000
. : milestone, 14847000,
appsec (14.621 s) : 14621000, 14621000
. : milestone, 14621000,
iast (18.318 s) : 18318000, 18318000
. : milestone, 18318000,
iast_GLOBAL (18.278 s) : 18278000, 18278000
. : milestone, 18278000,
profiling (14.719 s) : 14719000, 14719000
. : milestone, 14719000,
tracing (14.959 s) : 14959000, 14959000
. : milestone, 14959000,
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.61.0-SNAPSHOT~9e0bebb6dd, baseline=1.61.0-SNAPSHOT~aa7c70f2e7
dateFormat X
axisFormat %s
section baseline
no_agent (1.498 ms) : 1487, 1510
. : milestone, 1498,
appsec (3.868 ms) : 3643, 4094
. : milestone, 3868,
iast (2.288 ms) : 2219, 2358
. : milestone, 2288,
iast_GLOBAL (2.329 ms) : 2259, 2399
. : milestone, 2329,
profiling (2.102 ms) : 2047, 2157
. : milestone, 2102,
tracing (2.096 ms) : 2043, 2150
. : milestone, 2096,
section candidate
no_agent (1.497 ms) : 1485, 1508
. : milestone, 1497,
appsec (3.845 ms) : 3622, 4067
. : milestone, 3845,
iast (2.288 ms) : 2219, 2358
. : milestone, 2288,
iast_GLOBAL (2.338 ms) : 2268, 2408
. : milestone, 2338,
profiling (2.109 ms) : 2055, 2164
. : milestone, 2109,
tracing (2.104 ms) : 2051, 2158
. : milestone, 2104,
|
Contributor
PerfectSlayer
left a comment
PerfectSlayer
left a comment
There was a problem hiding this comment.
Keep adding comments.
About the span link test case, I can help to rework it once you're ready to review as I have the context.
| "link after start only | false | true ", | ||
| "links before and after | true | true " | ||
| }) | ||
| @ParameterizedTest(name = "add span link at any time [{index}]") |
Contributor
There was a problem hiding this comment.
🎯 suggestion: Same suggestion about index vs scenario
Suggested change
| @ParameterizedTest(name = "add span link at any time [{index}]") |
| @ParameterizedTest(name = "add span link at any time [{index}]") | ||
| void addSpanLinkAtAnyTime(boolean beforeStart, boolean afterStart) throws Exception { | ||
| AgentTracer.SpanBuilder builder = tracer.buildSpan("test", "operation"); | ||
| List<SpanLink> links = new java.util.ArrayList<>(); |
Contributor
There was a problem hiding this comment.
🎯 suggestion: Use import
| .getTypeFactory() | ||
| .constructCollectionType(List.class, TestSpanLinkJson.class)); | ||
|
|
||
| assertEquals((beforeStart ? 1 : 0) + (afterStart ? 1 : 0), decodedSpanLinks.size()); |
Contributor
There was a problem hiding this comment.
🎯 suggestion:
Suggested change
| assertEquals((beforeStart ? 1 : 0) + (afterStart ? 1 : 0), decodedSpanLinks.size()); | |
| int expectedLinkCount = (beforeStart ? 1 : 0) + (afterStart ? 1 : 0); | |
| assertEquals(expectedLinkCount, decodedSpanLinks.size()); |
| Map<String, String> attributes = new HashMap<>(); | ||
| attributes.put("link-index", Integer.toString(index)); | ||
|
|
||
| return new DDSpanLink( |
Contributor
There was a problem hiding this comment.
❔ question: Should we have an implementation of SpanLink for tests that will open the constructor? Or hide it behind a static helper/constructor. But DDSpanLink don't seem right here.
Member
Author
There was a problem hiding this comment.
up to you here. you can do some refactoring afterward.
to me, does feel right. I don't see any issue.
| } | ||
| } | ||
|
|
||
| static class TestSpanLinkJson { |
Contributor
There was a problem hiding this comment.
🎯 suggestion: SpanLinkAsTag would better express the concern and the reason of the JSON encoding.
| protected boolean assertThreadsEachCleanup = true; | ||
| private boolean ignoreThreadCleanup; | ||
|
|
||
| static void allowContextTesting() { |
Contributor
There was a problem hiding this comment.
💭 thought: This will be a duplicate of the already merge feature here: https://github.com/DataDog/dd-trace-java/pull/11009/changes#diff-64b7ed73da5a4f2c4e2f4c285b90359f2f1be89b41dc4053377a0ac1eebe169f
We need to refactor it later.
migrate DDSpecification and DDCoreSpecification that are used by most of the test in this module. we are keeping the groovy version to be able to incrementally migrate tests. a first small test (DDSpanLinkTest )is migrated to prove the viability of the strategy.
jpbempel
force-pushed
the
jpbempel/g2j-DDSpec
branch
from
4e01627 to
9e0bebb
Compare
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. |
jpbempel
added
comp: testing
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
2 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
migrate DDSpecification and DDCoreSpecification that are used by most of the test in this module.
we are keeping the groovy version to be able to incrementally migrate tests.
a first small test (DDSpanLinkTest )is migrated to prove the viability of the strategy.
Motivation
this is part of the effort to migrate groovy tests to Java/JUnit
part1: #11053
Additional Notes
Key design decisions for DDJavaSpecification:
@TestInstance(Lifecycle.PER_CLASS)— allows non-static@BeforeAll/@AfterAllmethods,mirrors Spock's per-class lifecycle where
setupSpec/cleanupSpecrun once per test classinstallConfigTransformer()in the static block — handles the ByteBuddy transformation(equivalent to
ConfigTransformSpockExtension) inline, makingConfig.INSTANCEpublic/static/volatile before
makeConfigInstanceModifiable()runsTestEnvironmentVariablesinner class — Java port ofControllableEnvironmentVariables(whichis Groovy and compiles after Java, so can't be referenced from Java source)
configModificationFailedis static package-private soConfigInstrumentationFailedListener(inner class) can access it
@BeforeAll setupSpec(),@AfterAll cleanupSpec(),@BeforeEach setup(),@AfterEach cleanup()— same semantics as SpockinjectSysConfig,injectEnvConfig,rebuildConfig, etc.) arepresent with Java overload pattern replacing Groovy's default parameters
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.