Uh oh!
There was an error while loading. Please reload this page.
Improve Baggage API - #8523
Conversation
There was a problem hiding this comment.
static final vars are considered as constants and are uppercased by convention
There was a problem hiding this comment.
I admit to having mixed views on this - I understand the convention of marking static final's as upper-cased constants, but find using LOG.info(...) rather glaring in the code.
Do we want to codify this in the spotless config and update all the code to use this convention, or just follow it for new files?
There was a problem hiding this comment.
find using LOG.info(...) rather glaring in the code.
Interesting. What's the rationale behind your feeling?
LOG being a constant, it makes sense to me.
During reviews, I usually end up looking for the definition of anything I can't find in the immediate scope that is not a constant (UPPER_CASE) nor a field member (this.field).
There was a problem hiding this comment.
Past habit of distinguishing between value constants (denoting a fixed simple value) and singletons (which are similar in that the reference doesn't change, but internally things vary.)
I'm in no way consistent about this though, having picked up the use of INSTANCE while working in this codebase :)
There was a problem hiding this comment.
Don't worry about the this. additions, it's mostly my formatter at play here and there...
There was a problem hiding this comment.
Moved from BaggageContext to Baggage. Context is not a concept the API user will be concerned about. It will be mostly interested in the key/value capabilites.
There was a problem hiding this comment.
I added Javadoc for all public class / methods as they are part of the API.
The goal is to improve discoverability, describe what it's supposed to do, and give hint about specific implementation logic.
There was a problem hiding this comment.
From baggageString to w3cHeader: this will highlight the logic at play (the encoding).
baggagedon't give clue in aBaggageContextclassStringis already the type of var
There was a problem hiding this comment.
updatedCache is gone too. As the string can be null, we can convey that there is no more "cached" header value if it's null. This simplifies the getter too.
There was a problem hiding this comment.
All static factory method are moved at top of the class by convention. (usually the first methods the API users will interact with to get an instance)
There was a problem hiding this comment.
Documented it can return null. Null safety is an issue with Java lang... I added @Null too in the API to help the IDE to warn the API user.
There was a problem hiding this comment.
Moved it as a static method as it missing.
This is where you see that some tests might be missing from the internal-api module.
There was a problem hiding this comment.
I end up adding unit tests :)
There was a problem hiding this comment.
I introduce "item" concept here too.
You already used it in the propagator code:
// if there are already baggage items processed, add and allocate bytes for a comma
// reached the max number of baggage items allowed
There was a problem hiding this comment.
I only kept one constructor as it serves it internally initialize the object final field.
As they are not supposed to use the constructor (but static factories instead), it's fine to only have one that do all fields at once.
There was a problem hiding this comment.
This is an optimization where you will only void the cache if it effectively alter the items map
There was a problem hiding this comment.
Change to read-only view rather than defensive copy.
It will reduce the memory usage during injection.
The Javadoc will help to explicit the expected behavior here.
There was a problem hiding this comment.
I went with a very basic structure like object.
As there is no logic here, all fields are opened.
Only left an helper constructor to create the object with the right value in a single line.
This whole object thing is a not great but a general mitigation of Java lang as not being able to return multiple values.
I also renamed it to avoid having call like data.data = xxx.
There was a problem hiding this comment.
Extracted few constants as class constant for clarity, avoiding magic string within method body.
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 59 metrics, 4 unstable metrics. Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.48.0-SNAPSHOT~477151989b, baseline=1.48.0-SNAPSHOT~1dff23cfc8
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.043 s) : 0, 1042645
Total [baseline] (10.481 s) : 0, 10480962
Agent [candidate] (1.04 s) : 0, 1040139
Total [candidate] (10.488 s) : 0, 10487725
section appsec
Agent [baseline] (1.184 s) : 0, 1184318
Total [baseline] (10.788 s) : 0, 10787820
Agent [candidate] (1.183 s) : 0, 1183087
Total [candidate] (10.755 s) : 0, 10754521
section iast
Agent [baseline] (1.169 s) : 0, 1169179
Total [baseline] (10.957 s) : 0, 10957442
Agent [candidate] (1.173 s) : 0, 1172729
Total [candidate] (10.959 s) : 0, 10958522
section profiling
Agent [baseline] (1.263 s) : 0, 1262887
Total [baseline] (10.845 s) : 0, 10845196
Agent [candidate] (1.262 s) : 0, 1262372
Total [candidate] (10.893 s) : 0, 10893477
gantt
title petclinic - break down per module: candidate=1.48.0-SNAPSHOT~477151989b, baseline=1.48.0-SNAPSHOT~1dff23cfc8
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (719.609 ms) : 0, 719609
BytebuddyAgent [candidate] (717.998 ms) : 0, 717998
GlobalTracer [baseline] (240.196 ms) : 0, 240196
GlobalTracer [candidate] (239.972 ms) : 0, 239972
AppSec [baseline] (55.121 ms) : 0, 55121
AppSec [candidate] (55.089 ms) : 0, 55089
Remote Config [baseline] (685.916 µs) : 0, 686
Remote Config [candidate] (691.279 µs) : 0, 691
Telemetry [baseline] (12.236 ms) : 0, 12236
Telemetry [candidate] (11.567 ms) : 0, 11567
section appsec
BytebuddyAgent [baseline] (736.414 ms) : 0, 736414
BytebuddyAgent [candidate] (736.195 ms) : 0, 736195
GlobalTracer [baseline] (236.765 ms) : 0, 236765
GlobalTracer [candidate] (236.188 ms) : 0, 236188
AppSec [baseline] (176.388 ms) : 0, 176388
AppSec [candidate] (176.283 ms) : 0, 176283
Remote Config [baseline] (668.955 µs) : 0, 669
Remote Config [candidate] (660.016 µs) : 0, 660
Telemetry [baseline] (8.309 ms) : 0, 8309
Telemetry [candidate] (8.248 ms) : 0, 8248
IAST [baseline] (21.707 ms) : 0, 21707
IAST [candidate] (21.374 ms) : 0, 21374
section iast
BytebuddyAgent [baseline] (835.519 ms) : 0, 835519
BytebuddyAgent [candidate] (838.322 ms) : 0, 838322
GlobalTracer [baseline] (229.905 ms) : 0, 229905
GlobalTracer [candidate] (230.686 ms) : 0, 230686
AppSec [baseline] (56.8 ms) : 0, 56800
AppSec [candidate] (55.893 ms) : 0, 55893
Remote Config [baseline] (610.549 µs) : 0, 611
Remote Config [candidate] (610.161 µs) : 0, 610
Telemetry [baseline] (8.66 ms) : 0, 8660
Telemetry [candidate] (8.748 ms) : 0, 8748
IAST [baseline] (22.853 ms) : 0, 22853
IAST [candidate] (23.604 ms) : 0, 23604
section profiling
ProfilingAgent [baseline] (96.081 ms) : 0, 96081
ProfilingAgent [candidate] (96.159 ms) : 0, 96159
BytebuddyAgent [baseline] (710.907 ms) : 0, 710907
BytebuddyAgent [candidate] (710.628 ms) : 0, 710628
GlobalTracer [baseline] (350.57 ms) : 0, 350570
GlobalTracer [candidate] (351.21 ms) : 0, 351210
AppSec [baseline] (55.278 ms) : 0, 55278
AppSec [candidate] (54.332 ms) : 0, 54332
Remote Config [baseline] (754.775 µs) : 0, 755
Remote Config [candidate] (673.636 µs) : 0, 674
Telemetry [baseline] (9.013 ms) : 0, 9013
Telemetry [candidate] (8.935 ms) : 0, 8935
Profiling [baseline] (96.106 ms) : 0, 96106
Profiling [candidate] (96.186 ms) : 0, 96186
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.48.0-SNAPSHOT~477151989b, baseline=1.48.0-SNAPSHOT~1dff23cfc8
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.049 s) : 0, 1049338
Total [baseline] (8.734 s) : 0, 8734291
Agent [candidate] (1.047 s) : 0, 1047285
Total [candidate] (8.696 s) : 0, 8695919
section iast
Agent [baseline] (1.171 s) : 0, 1170645
Total [baseline] (9.237 s) : 0, 9236962
Agent [candidate] (1.176 s) : 0, 1175670
Total [candidate] (9.294 s) : 0, 9294441
section iast_HARDCODED_SECRET_DISABLED
Agent [baseline] (1.181 s) : 0, 1180529
Total [baseline] (9.203 s) : 0, 9202507
Agent [candidate] (1.176 s) : 0, 1175695
Total [candidate] (9.221 s) : 0, 9220731
section iast_TELEMETRY_OFF
Agent [baseline] (1.174 s) : 0, 1173546
Total [baseline] (9.306 s) : 0, 9305894
Agent [candidate] (1.167 s) : 0, 1167300
Total [candidate] (9.241 s) : 0, 9240515
gantt
title insecure-bank - break down per module: candidate=1.48.0-SNAPSHOT~477151989b, baseline=1.48.0-SNAPSHOT~1dff23cfc8
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (724.279 ms) : 0, 724279
BytebuddyAgent [candidate] (723.408 ms) : 0, 723408
GlobalTracer [baseline] (242.346 ms) : 0, 242346
GlobalTracer [candidate] (241.167 ms) : 0, 241167
AppSec [baseline] (55.476 ms) : 0, 55476
AppSec [candidate] (55.463 ms) : 0, 55463
Remote Config [baseline] (706.143 µs) : 0, 706
Remote Config [candidate] (705.898 µs) : 0, 706
Telemetry [baseline] (11.583 ms) : 0, 11583
Telemetry [candidate] (11.61 ms) : 0, 11610
section iast
BytebuddyAgent [baseline] (837.108 ms) : 0, 837108
BytebuddyAgent [candidate] (839.339 ms) : 0, 839339
GlobalTracer [baseline] (229.807 ms) : 0, 229807
GlobalTracer [candidate] (232.048 ms) : 0, 232048
IAST [baseline] (22.829 ms) : 0, 22829
IAST [candidate] (22.889 ms) : 0, 22889
AppSec [baseline] (56.716 ms) : 0, 56716
AppSec [candidate] (57.178 ms) : 0, 57178
Remote Config [baseline] (616.384 µs) : 0, 616
Remote Config [candidate] (624.597 µs) : 0, 625
Telemetry [baseline] (8.749 ms) : 0, 8749
Telemetry [candidate] (8.756 ms) : 0, 8756
section iast_HARDCODED_SECRET_DISABLED
BytebuddyAgent [baseline] (843.581 ms) : 0, 843581
BytebuddyAgent [candidate] (840.703 ms) : 0, 840703
GlobalTracer [baseline] (231.818 ms) : 0, 231818
GlobalTracer [candidate] (231.047 ms) : 0, 231047
IAST [baseline] (23.208 ms) : 0, 23208
IAST [candidate] (22.839 ms) : 0, 22839
AppSec [baseline] (57.364 ms) : 0, 57364
AppSec [candidate] (56.876 ms) : 0, 56876
Remote Config [baseline] (627.031 µs) : 0, 627
Remote Config [candidate] (617.854 µs) : 0, 618
Telemetry [baseline] (8.9 ms) : 0, 8900
Telemetry [candidate] (8.701 ms) : 0, 8701
section iast_TELEMETRY_OFF
BytebuddyAgent [baseline] (839.757 ms) : 0, 839757
BytebuddyAgent [candidate] (834.115 ms) : 0, 834115
GlobalTracer [baseline] (229.953 ms) : 0, 229953
GlobalTracer [candidate] (230.205 ms) : 0, 230205
IAST [baseline] (22.573 ms) : 0, 22573
IAST [candidate] (22.287 ms) : 0, 22287
AppSec [baseline] (56.994 ms) : 0, 56994
AppSec [candidate] (56.68 ms) : 0, 56680
Remote Config [baseline] (620.356 µs) : 0, 620
Remote Config [candidate] (613.247 µs) : 0, 613
Telemetry [baseline] (8.724 ms) : 0, 8724
Telemetry [candidate] (8.51 ms) : 0, 8510
LoadParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 13 metrics, 17 unstable metrics. Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.48.0-SNAPSHOT~477151989b, baseline=1.48.0-SNAPSHOT~1dff23cfc8
dateFormat X
axisFormat %s
section baseline
no_agent (379.656 µs) : 360, 400
. : milestone, 380,
iast (514.953 µs) : 493, 537
. : milestone, 515,
iast_FULL (729.828 µs) : 708, 752
. : milestone, 730,
iast_GLOBAL (563.758 µs) : 540, 587
. : milestone, 564,
iast_HARDCODED_SECRET_DISABLED (516.846 µs) : 495, 539
. : milestone, 517,
iast_INACTIVE (468.022 µs) : 446, 490
. : milestone, 468,
iast_TELEMETRY_OFF (505.118 µs) : 482, 528
. : milestone, 505,
tracing (462.502 µs) : 441, 484
. : milestone, 463,
section candidate
no_agent (381.941 µs) : 362, 402
. : milestone, 382,
iast (510.289 µs) : 488, 532
. : milestone, 510,
iast_FULL (732.998 µs) : 711, 755
. : milestone, 733,
iast_GLOBAL (574.288 µs) : 551, 598
. : milestone, 574,
iast_HARDCODED_SECRET_DISABLED (519.044 µs) : 497, 541
. : milestone, 519,
iast_INACTIVE (472.755 µs) : 451, 494
. : milestone, 473,
iast_TELEMETRY_OFF (506.464 µs) : 485, 528
. : milestone, 506,
tracing (470.239 µs) : 448, 492
. : milestone, 470,
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.48.0-SNAPSHOT~477151989b, baseline=1.48.0-SNAPSHOT~1dff23cfc8
dateFormat X
axisFormat %s
section baseline
no_agent (1.348 ms) : 1328, 1368
. : milestone, 1348,
appsec (1.743 ms) : 1719, 1767
. : milestone, 1743,
appsec_no_iast (1.749 ms) : 1724, 1774
. : milestone, 1749,
code_origins (1.678 ms) : 1650, 1706
. : milestone, 1678,
iast (1.513 ms) : 1489, 1538
. : milestone, 1513,
profiling (1.542 ms) : 1516, 1568
. : milestone, 1542,
tracing (1.495 ms) : 1469, 1521
. : milestone, 1495,
section candidate
no_agent (1.358 ms) : 1338, 1378
. : milestone, 1358,
appsec (1.74 ms) : 1716, 1764
. : milestone, 1740,
appsec_no_iast (1.727 ms) : 1702, 1753
. : milestone, 1727,
code_origins (1.677 ms) : 1650, 1705
. : milestone, 1677,
iast (1.511 ms) : 1486, 1536
. : milestone, 1511,
profiling (1.556 ms) : 1532, 1580
. : milestone, 1556,
tracing (1.501 ms) : 1476, 1526
. : milestone, 1501,
DacapoParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 12 metrics, 0 unstable metrics. Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.48.0-SNAPSHOT~477151989b, baseline=1.48.0-SNAPSHOT~1dff23cfc8
dateFormat X
axisFormat %s
section baseline
no_agent (14.643 s) : 14643000, 14643000
. : milestone, 14643000,
appsec (15.051 s) : 15051000, 15051000
. : milestone, 15051000,
iast (18.617 s) : 18617000, 18617000
. : milestone, 18617000,
iast_GLOBAL (18.009 s) : 18009000, 18009000
. : milestone, 18009000,
profiling (15.034 s) : 15034000, 15034000
. : milestone, 15034000,
tracing (14.681 s) : 14681000, 14681000
. : milestone, 14681000,
section candidate
no_agent (14.969 s) : 14969000, 14969000
. : milestone, 14969000,
appsec (14.916 s) : 14916000, 14916000
. : milestone, 14916000,
iast (19.108 s) : 19108000, 19108000
. : milestone, 19108000,
iast_GLOBAL (17.936 s) : 17936000, 17936000
. : milestone, 17936000,
profiling (15.478 s) : 15478000, 15478000
. : milestone, 15478000,
tracing (14.841 s) : 14841000, 14841000
. : milestone, 14841000,
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.48.0-SNAPSHOT~477151989b, baseline=1.48.0-SNAPSHOT~1dff23cfc8
dateFormat X
axisFormat %s
section baseline
no_agent (1.475 ms) : 1463, 1486
. : milestone, 1475,
appsec (2.339 ms) : 2295, 2383
. : milestone, 2339,
iast (2.123 ms) : 2067, 2179
. : milestone, 2123,
iast_GLOBAL (2.156 ms) : 2099, 2212
. : milestone, 2156,
profiling (1.959 ms) : 1915, 2003
. : milestone, 1959,
tracing (1.949 ms) : 1906, 1993
. : milestone, 1949,
section candidate
no_agent (1.478 ms) : 1467, 1490
. : milestone, 1478,
appsec (2.347 ms) : 2303, 2391
. : milestone, 2347,
iast (2.13 ms) : 2074, 2186
. : milestone, 2130,
iast_GLOBAL (2.163 ms) : 2107, 2219
. : milestone, 2163,
profiling (1.964 ms) : 1920, 2008
. : milestone, 1964,
tracing (1.96 ms) : 1917, 2003
. : milestone, 1960,
|
4413ea0 to
67922cdCompare
mhlidd
left a comment
There was a problem hiding this comment.
Some small nit comments/questions. Otherwise makes sense to me!
There was a problem hiding this comment.
Should we change the log message to match the new Baggage class?
There was a problem hiding this comment.
Yup, will fix it. Good catch!
Actually, there is an option when refactoring class name to include "code" or "code and comment". I should have picked the wrong one as the IDE missed them 😓
There was a problem hiding this comment.
Would not checking for null here potentially lead to a NullPointerException?
There was a problem hiding this comment.
Interesting question. This changes was made to simplify the checke and denotes a common rule for Java object equality check: if you have value V to test against a constant C, you would better write if (C.equals(V)) { rather than if (V.equals(C)) {.
While equals implementation contract enforces to have A.equals(B) and B.equals(A) to give the same result, they don't behave the same with null value. As equals is an instance method, it will only throw an NPE if the left operator (the instance object on which method is called) is null not the right operator (the call parameter).
So you end up always writing C.equals(V) because the constant is usually not null and save you from future NPE.
There was a problem hiding this comment.
Oh interesting, I never knew that but it makes sense!
67922cd to
cee3751CompareUh oh!
There was an error while loading. Please reload this page.
cee3751 to
4771519Compare* feat(internal-api): Improve Baggage API * feat(internal-api): Add Baggage unit tests
What Does This Do
Improve the new Baggage API by improving Javadoc and semantic and add unit tests.
Motivation
This is a quick feedback as part of mentorship.
@mhlidd your turn to do the review 😉
Additional Notes
Contributor Checklist
type:and (comp:orinst:) labels in addition to any usefull labelsclose,fixor any linking keywords when referencing an issue.Use
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]