-
Notifications
You must be signed in to change notification settings - Fork 323
Remove RadixTreeCache for int status and protect httpStatus short cache #10236
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
| } catch (Throwable t) { | ||
| // RadixTreeCache seems to have issues on semeru11 - NPE on AtomicReferenceArray cas | ||
| // skip the cache in those cases and just create a String | ||
| return UTF8BytesString.create(Short.toString(httpStatusCode)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Will this create too many allocations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not supposed to fail the cache acccess. Note that this method is on the hot path when finish is called on the root span -> write is called. If this throw we might bubble the application code IMO. Better to allocate something than throw
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the failure constant on Semeru? Or is it sporadic?
If it is constant, we should probably detect the failure and then just bypass the cache there after.
I've started debating using UTF8BytesString for values, since we now have the UTF8 cache. I also have some evidence that some caches are doing more harm than good now.
Long term, I'm hoping that TagMap will provide a more elegant solution to these problems, but unfortunately, I'm not there yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well I think more that there are no failures in the real life but only happens in CI. The stacktrace is very weird since it sounds like implying that either unsafe is null (sounds impossible) either the array is itself is null (that sounds not possible as well unless the class is unsafely allocated). In CI it passes on a retry run on a fresh JVM that looks indicating that something is gets corrupted on the used jvm? I put this catch in a defensive way - I can remove if needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How many semeru 11 hosts can be impacted ?
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 56 metrics, 9 unstable metrics. Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.58.0-SNAPSHOT~0d51cb0ba3, baseline=1.58.0-SNAPSHOT~5627283442
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.09 s) : 0, 1090257
Total [baseline] (8.806 s) : 0, 8806426
Agent [candidate] (1.093 s) : 0, 1092692
Total [candidate] (8.75 s) : 0, 8749878
section iast
Agent [baseline] (1.228 s) : 0, 1227677
Total [baseline] (9.303 s) : 0, 9302599
Agent [candidate] (1.219 s) : 0, 1218555
Total [candidate] (9.267 s) : 0, 9267287
gantt
title insecure-bank - break down per module: candidate=1.58.0-SNAPSHOT~0d51cb0ba3, baseline=1.58.0-SNAPSHOT~5627283442
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.196 ms) : 0, 1196
crashtracking [candidate] (1.192 ms) : 0, 1192
BytebuddyAgent [baseline] (655.112 ms) : 0, 655112
BytebuddyAgent [candidate] (656.537 ms) : 0, 656537
GlobalTracer [baseline] (283.92 ms) : 0, 283920
GlobalTracer [candidate] (284.343 ms) : 0, 284343
AppSec [baseline] (33.037 ms) : 0, 33037
AppSec [candidate] (33.069 ms) : 0, 33069
Debugger [baseline] (67.796 ms) : 0, 67796
Debugger [candidate] (68.277 ms) : 0, 68277
Remote Config [baseline] (651.024 µs) : 0, 651
Remote Config [candidate] (633.827 µs) : 0, 634
Telemetry [baseline] (9.098 ms) : 0, 9098
Telemetry [candidate] (9.136 ms) : 0, 9136
Flare Poller [baseline] (3.819 ms) : 0, 3819
Flare Poller [candidate] (3.811 ms) : 0, 3811
section iast
crashtracking [baseline] (1.188 ms) : 0, 1188
crashtracking [candidate] (1.197 ms) : 0, 1197
BytebuddyAgent [baseline] (795.52 ms) : 0, 795520
BytebuddyAgent [candidate] (788.432 ms) : 0, 788432
GlobalTracer [baseline] (256.592 ms) : 0, 256592
GlobalTracer [candidate] (255.118 ms) : 0, 255118
IAST [baseline] (27.13 ms) : 0, 27130
IAST [candidate] (26.837 ms) : 0, 26837
AppSec [baseline] (33.645 ms) : 0, 33645
AppSec [candidate] (34.295 ms) : 0, 34295
Debugger [baseline] (65.365 ms) : 0, 65365
Debugger [candidate] (64.712 ms) : 0, 64712
Remote Config [baseline] (615.313 µs) : 0, 615
Remote Config [candidate] (624.832 µs) : 0, 625
Telemetry [baseline] (8.415 ms) : 0, 8415
Telemetry [candidate] (8.461 ms) : 0, 8461
Flare Poller [baseline] (3.586 ms) : 0, 3586
Flare Poller [candidate] (3.564 ms) : 0, 3564
Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.58.0-SNAPSHOT~0d51cb0ba3, baseline=1.58.0-SNAPSHOT~5627283442
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.088 s) : 0, 1088189
Total [baseline] (10.744 s) : 0, 10743975
Agent [candidate] (1.088 s) : 0, 1088023
Total [candidate] (10.874 s) : 0, 10873538
section appsec
Agent [baseline] (1.265 s) : 0, 1264760
Total [baseline] (10.875 s) : 0, 10875086
Agent [candidate] (1.271 s) : 0, 1271306
Total [candidate] (10.992 s) : 0, 10992138
section iast
Agent [baseline] (1.221 s) : 0, 1220597
Total [baseline] (11.153 s) : 0, 11152543
Agent [candidate] (1.222 s) : 0, 1222479
Total [candidate] (11.17 s) : 0, 11169563
section profiling
Agent [baseline] (1.208 s) : 0, 1208070
Total [baseline] (10.846 s) : 0, 10846239
Agent [candidate] (1.209 s) : 0, 1208853
Total [candidate] (10.987 s) : 0, 10986783
gantt
title petclinic - break down per module: candidate=1.58.0-SNAPSHOT~0d51cb0ba3, baseline=1.58.0-SNAPSHOT~5627283442
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.19 ms) : 0, 1190
crashtracking [candidate] (1.206 ms) : 0, 1206
BytebuddyAgent [baseline] (653.794 ms) : 0, 653794
BytebuddyAgent [candidate] (652.14 ms) : 0, 652140
GlobalTracer [baseline] (283.544 ms) : 0, 283544
GlobalTracer [candidate] (284.119 ms) : 0, 284119
AppSec [baseline] (32.751 ms) : 0, 32751
AppSec [candidate] (32.865 ms) : 0, 32865
Debugger [baseline] (67.996 ms) : 0, 67996
Debugger [candidate] (67.914 ms) : 0, 67914
Remote Config [baseline] (606.726 µs) : 0, 607
Remote Config [candidate] (626.619 µs) : 0, 627
Telemetry [baseline] (9.01 ms) : 0, 9010
Telemetry [candidate] (9.062 ms) : 0, 9062
Flare Poller [baseline] (3.721 ms) : 0, 3721
Flare Poller [candidate] (4.539 ms) : 0, 4539
section appsec
crashtracking [baseline] (1.179 ms) : 0, 1179
crashtracking [candidate] (1.186 ms) : 0, 1186
BytebuddyAgent [baseline] (690.889 ms) : 0, 690889
BytebuddyAgent [candidate] (695.311 ms) : 0, 695311
GlobalTracer [baseline] (257.882 ms) : 0, 257882
GlobalTracer [candidate] (259.002 ms) : 0, 259002
AppSec [baseline] (174.535 ms) : 0, 174535
AppSec [candidate] (174.621 ms) : 0, 174621
Debugger [baseline] (66.519 ms) : 0, 66519
Debugger [candidate] (66.917 ms) : 0, 66917
Remote Config [baseline] (743.371 µs) : 0, 743
Remote Config [candidate] (682.904 µs) : 0, 683
Telemetry [baseline] (9.253 ms) : 0, 9253
Telemetry [candidate] (9.593 ms) : 0, 9593
Flare Poller [baseline] (3.754 ms) : 0, 3754
Flare Poller [candidate] (3.764 ms) : 0, 3764
IAST [baseline] (24.622 ms) : 0, 24622
IAST [candidate] (24.758 ms) : 0, 24758
section iast
crashtracking [baseline] (1.177 ms) : 0, 1177
crashtracking [candidate] (1.176 ms) : 0, 1176
BytebuddyAgent [baseline] (789.787 ms) : 0, 789787
BytebuddyAgent [candidate] (790.393 ms) : 0, 790393
GlobalTracer [baseline] (255.479 ms) : 0, 255479
GlobalTracer [candidate] (255.963 ms) : 0, 255963
AppSec [baseline] (34.127 ms) : 0, 34127
AppSec [candidate] (33.622 ms) : 0, 33622
Debugger [baseline] (65.173 ms) : 0, 65173
Debugger [candidate] (66.233 ms) : 0, 66233
Remote Config [baseline] (598.886 µs) : 0, 599
Remote Config [candidate] (602.036 µs) : 0, 602
Telemetry [baseline] (8.439 ms) : 0, 8439
Telemetry [candidate] (8.525 ms) : 0, 8525
Flare Poller [baseline] (3.552 ms) : 0, 3552
Flare Poller [candidate] (3.583 ms) : 0, 3583
IAST [baseline] (26.953 ms) : 0, 26953
IAST [candidate] (27.042 ms) : 0, 27042
section profiling
ProfilingAgent [baseline] (96.913 ms) : 0, 96913
ProfilingAgent [candidate] (96.022 ms) : 0, 96022
crashtracking [baseline] (1.212 ms) : 0, 1212
crashtracking [candidate] (1.216 ms) : 0, 1216
BytebuddyAgent [baseline] (705.893 ms) : 0, 705893
BytebuddyAgent [candidate] (707.139 ms) : 0, 707139
GlobalTracer [baseline] (220.856 ms) : 0, 220856
GlobalTracer [candidate] (221.491 ms) : 0, 221491
AppSec [baseline] (32.252 ms) : 0, 32252
AppSec [candidate] (32.27 ms) : 0, 32270
Debugger [baseline] (67.921 ms) : 0, 67921
Debugger [candidate] (67.71 ms) : 0, 67710
Remote Config [baseline] (640.055 µs) : 0, 640
Remote Config [candidate] (623.862 µs) : 0, 624
Telemetry [baseline] (8.722 ms) : 0, 8722
Telemetry [candidate] (8.7 ms) : 0, 8700
Flare Poller [baseline] (3.67 ms) : 0, 3670
Flare Poller [candidate] (3.656 ms) : 0, 3656
Profiling [baseline] (97.479 ms) : 0, 97479
Profiling [candidate] (96.588 ms) : 0, 96588
LoadParameters
See matching parameters
SummaryFound 1 performance improvements and 1 performance regressions! Performance is the same for 18 metrics, 16 unstable metrics.
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.58.0-SNAPSHOT~0d51cb0ba3, baseline=1.58.0-SNAPSHOT~5627283442
dateFormat X
axisFormat %s
section baseline
no_agent (1.19 ms) : 1178, 1203
. : milestone, 1190,
iast (3.194 ms) : 3151, 3236
. : milestone, 3194,
iast_FULL (5.665 ms) : 5610, 5720
. : milestone, 5665,
iast_GLOBAL (3.574 ms) : 3523, 3626
. : milestone, 3574,
profiling (1.956 ms) : 1940, 1972
. : milestone, 1956,
tracing (1.781 ms) : 1767, 1796
. : milestone, 1781,
section candidate
no_agent (1.184 ms) : 1173, 1196
. : milestone, 1184,
iast (3.257 ms) : 3211, 3302
. : milestone, 3257,
iast_FULL (5.785 ms) : 5728, 5843
. : milestone, 5785,
iast_GLOBAL (3.629 ms) : 3575, 3683
. : milestone, 3629,
profiling (2.2 ms) : 2179, 2222
. : milestone, 2200,
tracing (1.783 ms) : 1768, 1798
. : milestone, 1783,
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.58.0-SNAPSHOT~0d51cb0ba3, baseline=1.58.0-SNAPSHOT~5627283442
dateFormat X
axisFormat %s
section baseline
no_agent (19.319 ms) : 19118, 19520
. : milestone, 19319,
appsec (18.946 ms) : 18756, 19137
. : milestone, 18946,
code_origins (17.884 ms) : 17705, 18063
. : milestone, 17884,
iast (17.462 ms) : 17288, 17637
. : milestone, 17462,
profiling (18.457 ms) : 18275, 18640
. : milestone, 18457,
tracing (17.789 ms) : 17614, 17963
. : milestone, 17789,
section candidate
no_agent (17.059 ms) : 16887, 17231
. : milestone, 17059,
appsec (19.378 ms) : 19179, 19577
. : milestone, 19378,
code_origins (18.087 ms) : 17906, 18269
. : milestone, 18087,
iast (17.408 ms) : 17238, 17578
. : milestone, 17408,
profiling (19.05 ms) : 18859, 19241
. : milestone, 19050,
tracing (17.861 ms) : 17684, 18038
. : milestone, 17861,
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.58.0-SNAPSHOT~0d51cb0ba3, baseline=1.58.0-SNAPSHOT~5627283442
dateFormat X
axisFormat %s
section baseline
no_agent (15.648 s) : 15648000, 15648000
. : milestone, 15648000,
appsec (14.609 s) : 14609000, 14609000
. : milestone, 14609000,
iast (18.322 s) : 18322000, 18322000
. : milestone, 18322000,
iast_GLOBAL (17.646 s) : 17646000, 17646000
. : milestone, 17646000,
profiling (15.394 s) : 15394000, 15394000
. : milestone, 15394000,
tracing (14.794 s) : 14794000, 14794000
. : milestone, 14794000,
section candidate
no_agent (15.635 s) : 15635000, 15635000
. : milestone, 15635000,
appsec (14.683 s) : 14683000, 14683000
. : milestone, 14683000,
iast (18.258 s) : 18258000, 18258000
. : milestone, 18258000,
iast_GLOBAL (17.833 s) : 17833000, 17833000
. : milestone, 17833000,
profiling (14.926 s) : 14926000, 14926000
. : milestone, 14926000,
tracing (14.721 s) : 14721000, 14721000
. : milestone, 14721000,
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.58.0-SNAPSHOT~0d51cb0ba3, baseline=1.58.0-SNAPSHOT~5627283442
dateFormat X
axisFormat %s
section baseline
no_agent (1.474 ms) : 1463, 1486
. : milestone, 1474,
appsec (3.665 ms) : 3449, 3880
. : milestone, 3665,
iast (2.221 ms) : 2156, 2286
. : milestone, 2221,
iast_GLOBAL (2.269 ms) : 2203, 2335
. : milestone, 2269,
profiling (2.087 ms) : 2033, 2142
. : milestone, 2087,
tracing (2.047 ms) : 1996, 2098
. : milestone, 2047,
section candidate
no_agent (1.474 ms) : 1463, 1486
. : milestone, 1474,
appsec (3.715 ms) : 3495, 3935
. : milestone, 3715,
iast (2.214 ms) : 2150, 2279
. : milestone, 2214,
iast_GLOBAL (2.256 ms) : 2191, 2321
. : milestone, 2256,
profiling (2.074 ms) : 2022, 2127
. : milestone, 2074,
tracing (2.063 ms) : 2011, 2114
. : milestone, 2063,
|
mcculls
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me, but I'd like @dougqh / platform team to give final approval
3934a15 to
0d51cb0
Compare
| measured, | ||
| topLevel, | ||
| httpStatusCode == 0 ? null : HTTP_STATUSES.get(httpStatusCode), | ||
| httpStatusCode == 0 ? null : shortStatusCodeToString(httpStatusCode), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious if we can use int here too? Same as you did with port?
Or it would be a huge refactoring?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed I think it can be just an int (I see we use a short indeed) since in other parts is casted as int
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to
| // http.status is important because it is expected to be a string downstream |
// http.status is important because it is expected to be a string downstream
so at this point in the metadata it should become a string I believe
What Does This Do
Recently, few tests in CI fails with semeru 11 because of:
This PR tries to alleviate this by:
Motivation
Additional Notes
Contributor Checklist
type:and (comp:orinst:) labels in addition to any useful labelsclose,fixor any linking keywords when referencing an issue.Use
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]