-
Notifications
You must be signed in to change notification settings - Fork 315
Error Logs Remediation 2 #9467
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
Error Logs Remediation 2 #9467
Conversation
Debugger benchmarksParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 10 metrics, 5 unstable metrics. See unchanged results
Request duration reports for reportsgantt
title reports - request duration [CI 0.99] : candidate=None, baseline=None
dateFormat X
axisFormat %s
section baseline
noprobe (314.707 µs) : 290, 339
. : milestone, 315,
basic (277.714 µs) : 272, 284
. : milestone, 278,
loop (8.974 ms) : 8969, 8979
. : milestone, 8974,
section candidate
noprobe (324.842 µs) : 286, 364
. : milestone, 325,
basic (282.771 µs) : 276, 290
. : milestone, 283,
loop (8.983 ms) : 8954, 9013
. : milestone, 8983,
|
|
🎯 Code Coverage 🔗 Commit SHA: 1038344 | Docs | Was this helpful? Give us feedback! |
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 50 metrics, 9 unstable metrics. Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.54.0-SNAPSHOT~103834421d, baseline=1.54.0-SNAPSHOT~2cfe50a636
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.066 s) : 0, 1066416
Total [baseline] (8.69 s) : 0, 8689824
Agent [candidate] (1.07 s) : 0, 1069584
Total [candidate] (8.621 s) : 0, 8621082
section iast
Agent [baseline] (1.191 s) : 0, 1191266
Total [baseline] (9.303 s) : 0, 9303239
Agent [candidate] (1.19 s) : 0, 1190098
Total [candidate] (9.282 s) : 0, 9281737
gantt
title insecure-bank - break down per module: candidate=1.54.0-SNAPSHOT~103834421d, baseline=1.54.0-SNAPSHOT~2cfe50a636
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.464 ms) : 0, 1464
crashtracking [candidate] (1.473 ms) : 0, 1473
BytebuddyAgent [baseline] (736.832 ms) : 0, 736832
BytebuddyAgent [candidate] (738.331 ms) : 0, 738331
GlobalTracer [baseline] (253.67 ms) : 0, 253670
GlobalTracer [candidate] (254.316 ms) : 0, 254316
AppSec [baseline] (30.762 ms) : 0, 30762
AppSec [candidate] (30.773 ms) : 0, 30773
Debugger [baseline] (6.465 ms) : 0, 6465
Debugger [candidate] (6.443 ms) : 0, 6443
Remote Config [baseline] (706.111 µs) : 0, 706
Remote Config [candidate] (704.231 µs) : 0, 704
Telemetry [baseline] (15.436 ms) : 0, 15436
Telemetry [candidate] (16.445 ms) : 0, 16445
section iast
crashtracking [baseline] (1.475 ms) : 0, 1475
crashtracking [candidate] (1.454 ms) : 0, 1454
BytebuddyAgent [baseline] (853.018 ms) : 0, 853018
BytebuddyAgent [candidate] (851.497 ms) : 0, 851497
GlobalTracer [baseline] (246.514 ms) : 0, 246514
GlobalTracer [candidate] (247.568 ms) : 0, 247568
IAST [baseline] (28.111 ms) : 0, 28111
IAST [candidate] (28.044 ms) : 0, 28044
AppSec [baseline] (26.345 ms) : 0, 26345
AppSec [candidate] (25.718 ms) : 0, 25718
Debugger [baseline] (6.009 ms) : 0, 6009
Debugger [candidate] (6.099 ms) : 0, 6099
Remote Config [baseline] (603.489 µs) : 0, 603
Remote Config [candidate] (600.214 µs) : 0, 600
Telemetry [baseline] (8.183 ms) : 0, 8183
Telemetry [candidate] (8.235 ms) : 0, 8235
Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.54.0-SNAPSHOT~103834421d, baseline=1.54.0-SNAPSHOT~2cfe50a636
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.062 s) : 0, 1062440
Total [baseline] (10.739 s) : 0, 10738605
Agent [candidate] (1.065 s) : 0, 1065314
Total [candidate] (10.746 s) : 0, 10745772
section appsec
Agent [baseline] (1.239 s) : 0, 1239113
Total [baseline] (11.112 s) : 0, 11111898
Agent [candidate] (1.234 s) : 0, 1234345
Total [candidate] (11.052 s) : 0, 11052333
section iast
Agent [baseline] (1.19 s) : 0, 1190419
Total [baseline] (11.072 s) : 0, 11072487
Agent [candidate] (1.192 s) : 0, 1191878
Total [candidate] (11.028 s) : 0, 11028324
section profiling
Agent [baseline] (1.214 s) : 0, 1214082
Total [baseline] (10.981 s) : 0, 10981468
Agent [candidate] (1.21 s) : 0, 1209769
Total [candidate] (11.182 s) : 0, 11181740
gantt
title petclinic - break down per module: candidate=1.54.0-SNAPSHOT~103834421d, baseline=1.54.0-SNAPSHOT~2cfe50a636
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.467 ms) : 0, 1467
crashtracking [candidate] (1.458 ms) : 0, 1458
BytebuddyAgent [baseline] (732.775 ms) : 0, 732775
BytebuddyAgent [candidate] (735.553 ms) : 0, 735553
GlobalTracer [baseline] (252.869 ms) : 0, 252869
GlobalTracer [candidate] (253.193 ms) : 0, 253193
AppSec [baseline] (30.585 ms) : 0, 30585
AppSec [candidate] (30.621 ms) : 0, 30621
Debugger [baseline] (6.396 ms) : 0, 6396
Debugger [candidate] (6.44 ms) : 0, 6440
Remote Config [baseline] (695.488 µs) : 0, 695
Remote Config [candidate] (697.032 µs) : 0, 697
Telemetry [baseline] (16.545 ms) : 0, 16545
Telemetry [candidate] (16.263 ms) : 0, 16263
section appsec
crashtracking [baseline] (1.459 ms) : 0, 1459
crashtracking [candidate] (1.458 ms) : 0, 1458
BytebuddyAgent [baseline] (759.403 ms) : 0, 759403
BytebuddyAgent [candidate] (755.745 ms) : 0, 755745
GlobalTracer [baseline] (246.572 ms) : 0, 246572
GlobalTracer [candidate] (245.806 ms) : 0, 245806
AppSec [baseline] (171.299 ms) : 0, 171299
AppSec [candidate] (171.336 ms) : 0, 171336
Debugger [baseline] (6.022 ms) : 0, 6022
Debugger [candidate] (5.964 ms) : 0, 5964
Remote Config [baseline] (630.617 µs) : 0, 631
Remote Config [candidate] (617.572 µs) : 0, 618
Telemetry [baseline] (8.556 ms) : 0, 8556
Telemetry [candidate] (8.477 ms) : 0, 8477
IAST [baseline] (23.919 ms) : 0, 23919
IAST [candidate] (23.765 ms) : 0, 23765
section iast
crashtracking [baseline] (1.463 ms) : 0, 1463
crashtracking [candidate] (1.452 ms) : 0, 1452
BytebuddyAgent [baseline] (852.415 ms) : 0, 852415
BytebuddyAgent [candidate] (853.678 ms) : 0, 853678
GlobalTracer [baseline] (245.703 ms) : 0, 245703
GlobalTracer [candidate] (246.263 ms) : 0, 246263
AppSec [baseline] (25.321 ms) : 0, 25321
AppSec [candidate] (26.065 ms) : 0, 26065
Debugger [baseline] (6.047 ms) : 0, 6047
Debugger [candidate] (6.033 ms) : 0, 6033
Remote Config [baseline] (602.002 µs) : 0, 602
Remote Config [candidate] (588.472 µs) : 0, 588
Telemetry [baseline] (8.193 ms) : 0, 8193
Telemetry [candidate] (8.136 ms) : 0, 8136
IAST [baseline] (29.687 ms) : 0, 29687
IAST [candidate] (28.703 ms) : 0, 28703
section profiling
ProfilingAgent [baseline] (107.767 ms) : 0, 107767
ProfilingAgent [candidate] (107.29 ms) : 0, 107290
crashtracking [baseline] (1.456 ms) : 0, 1456
crashtracking [candidate] (1.44 ms) : 0, 1440
BytebuddyAgent [baseline] (765.572 ms) : 0, 765572
BytebuddyAgent [candidate] (762.093 ms) : 0, 762093
GlobalTracer [baseline] (233.496 ms) : 0, 233496
GlobalTracer [candidate] (233.487 ms) : 0, 233487
AppSec [baseline] (30.57 ms) : 0, 30570
AppSec [candidate] (30.196 ms) : 0, 30196
Debugger [baseline] (11.241 ms) : 0, 11241
Debugger [candidate] (11.553 ms) : 0, 11553
Remote Config [baseline] (2.342 ms) : 0, 2342
Remote Config [candidate] (1.54 ms) : 0, 1540
Telemetry [baseline] (10.226 ms) : 0, 10226
Telemetry [candidate] (11.064 ms) : 0, 11064
Profiling [baseline] (108.432 ms) : 0, 108432
Profiling [candidate] (107.974 ms) : 0, 107974
LoadParameters
See matching parameters
SummaryFound 1 performance improvements and 0 performance regressions! Performance is the same for 11 metrics, 12 unstable metrics.
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.54.0-SNAPSHOT~103834421d, baseline=1.54.0-SNAPSHOT~2cfe50a636
dateFormat X
axisFormat %s
section baseline
no_agent (4.274 ms) : 4222, 4325
. : milestone, 4274,
iast (9.774 ms) : 9607, 9940
. : milestone, 9774,
iast_FULL (14.536 ms) : 14241, 14831
. : milestone, 14536,
iast_GLOBAL (10.804 ms) : 10608, 11000
. : milestone, 10804,
profiling (8.828 ms) : 8685, 8970
. : milestone, 8828,
tracing (7.815 ms) : 7703, 7926
. : milestone, 7815,
section candidate
no_agent (4.34 ms) : 4291, 4389
. : milestone, 4340,
iast (9.292 ms) : 9140, 9444
. : milestone, 9292,
iast_FULL (14.106 ms) : 13824, 14388
. : milestone, 14106,
iast_GLOBAL (10.832 ms) : 10640, 11025
. : milestone, 10832,
profiling (8.754 ms) : 8618, 8890
. : milestone, 8754,
tracing (7.892 ms) : 7765, 8019
. : milestone, 7892,
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.54.0-SNAPSHOT~103834421d, baseline=1.54.0-SNAPSHOT~2cfe50a636
dateFormat X
axisFormat %s
section baseline
no_agent (37.141 ms) : 36845, 37436
. : milestone, 37141,
appsec (46.222 ms) : 45827, 46617
. : milestone, 46222,
code_origins (45.725 ms) : 45334, 46116
. : milestone, 45725,
iast (44.609 ms) : 44229, 44989
. : milestone, 44609,
profiling (48.611 ms) : 48155, 49067
. : milestone, 48611,
tracing (45.146 ms) : 44768, 45525
. : milestone, 45146,
section candidate
no_agent (36.738 ms) : 36440, 37036
. : milestone, 36738,
appsec (47.404 ms) : 47009, 47799
. : milestone, 47404,
code_origins (46.004 ms) : 45621, 46388
. : milestone, 46004,
iast (44.289 ms) : 43910, 44668
. : milestone, 44289,
profiling (47.397 ms) : 46973, 47822
. : milestone, 47397,
tracing (45.445 ms) : 45046, 45845
. : milestone, 45445,
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.54.0-SNAPSHOT~103834421d, baseline=1.54.0-SNAPSHOT~2cfe50a636
dateFormat X
axisFormat %s
section baseline
no_agent (15.003 s) : 15003000, 15003000
. : milestone, 15003000,
appsec (14.76 s) : 14760000, 14760000
. : milestone, 14760000,
iast (18.072 s) : 18072000, 18072000
. : milestone, 18072000,
iast_GLOBAL (17.734 s) : 17734000, 17734000
. : milestone, 17734000,
profiling (15.872 s) : 15872000, 15872000
. : milestone, 15872000,
tracing (15.064 s) : 15064000, 15064000
. : milestone, 15064000,
section candidate
no_agent (15.259 s) : 15259000, 15259000
. : milestone, 15259000,
appsec (15.062 s) : 15062000, 15062000
. : milestone, 15062000,
iast (18.521 s) : 18521000, 18521000
. : milestone, 18521000,
iast_GLOBAL (17.954 s) : 17954000, 17954000
. : milestone, 17954000,
profiling (15.681 s) : 15681000, 15681000
. : milestone, 15681000,
tracing (14.968 s) : 14968000, 14968000
. : milestone, 14968000,
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.54.0-SNAPSHOT~103834421d, baseline=1.54.0-SNAPSHOT~2cfe50a636
dateFormat X
axisFormat %s
section baseline
no_agent (1.483 ms) : 1472, 1495
. : milestone, 1483,
appsec (3.7 ms) : 3484, 3916
. : milestone, 3700,
iast (2.21 ms) : 2147, 2273
. : milestone, 2210,
iast_GLOBAL (2.242 ms) : 2179, 2305
. : milestone, 2242,
profiling (2.044 ms) : 1993, 2095
. : milestone, 2044,
tracing (2.034 ms) : 1984, 2083
. : milestone, 2034,
section candidate
no_agent (1.48 ms) : 1468, 1491
. : milestone, 1480,
appsec (3.713 ms) : 3496, 3931
. : milestone, 3713,
iast (2.21 ms) : 2147, 2273
. : milestone, 2210,
iast_GLOBAL (2.245 ms) : 2182, 2308
. : milestone, 2245,
profiling (2.058 ms) : 2007, 2108
. : milestone, 2058,
tracing (2.03 ms) : 1981, 2080
. : milestone, 2030,
|
| ProbeDefinition definition = appliedDefinitions.get(encodedProbeId); | ||
| if (definition == null) { | ||
| LOGGER.warn(SEND_TELEMETRY, "Cannot resolve probe id=" + encodedProbeId); | ||
| LOGGER.warn(SEND_TELEMETRY, "Cannot resolve probe id={}", encodedProbeId); |
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.
this one is on purpose I want the probe ID sent to telemetry to help troubleshoot when we have this case!
this is not sensitive information. nothing related to the customer, this is a technical id
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.
Thanks! I'll roll back this one.
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, how concat and param are different here?
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, how concat and param are different here?
Only the format string "Cannot resolve probe id={}" is sent to the telemetry. Concatenation makes the otherwise hidden arguments available to the telemetry.
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.
Hmmm, we are hiding arguments to avoid sensitive data leakage? Correct?
If that true, I wish we have API for logging, that will support params and some flag managing what should be hidden... Something self-descriptive and better than concat... because you never know if this concat for purpose or just a human mistake...
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.
this one is on purpose I want the probe ID sent to telemetry to help troubleshoot when we have this case!
The logs that we send to the intake here are not really for normal troubleshooting and they are supposed to be constant message templates only so they can be properly deduplicated to reduce load.
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.
I agree that this could be addressed with an API change, but we currently use the standard Logger API backed by the TelemetryLogger, which reports errors to the backend.
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.
this one is on purpose I want the probe ID sent to telemetry to help troubleshoot when we have this case!
The logs that we send to the intake here are not really for normal troubleshooting and they are supposed to be constant message templates only so they can be properly deduplicated to reduce load.
Got it! Rolling back the rollback.
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.
those messages are cases, like exceptions.
If we cannot use them as telemetry for troubleshooting, they are useless!
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.
I agree. This should be allowed. Since the call is explicitly marked with SEND_TELEMETRY, we should permit sending the necessary additional information. To address concerns about cardinality, we should consider leveraging tags to capture this type of information.
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.
Nice!
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.
Thanks @ygree!
I think it is just two left from what I can find in GitHub:
- Debugger probe ID - I see there were comments on this one
- Profiler settings
- This was the main one that we discussed with profiling as they don't have access to this data in the flare, it'd be best to get sign off from someone on profiling @jbachorik (?) to see what we can do here.
| ProbeDefinition definition = appliedDefinitions.get(encodedProbeId); | ||
| if (definition == null) { | ||
| LOGGER.warn(SEND_TELEMETRY, "Cannot resolve probe id=" + encodedProbeId); | ||
| LOGGER.warn(SEND_TELEMETRY, "Cannot resolve probe id={}", encodedProbeId); |
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.
this one is on purpose I want the probe ID sent to telemetry to help troubleshoot when we have this case!
The logs that we send to the intake here are not really for normal troubleshooting and they are supposed to be constant message templates only so they can be properly deduplicated to reduce load.
ba11257 to
b9af27c
Compare
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.
I believe this change reveal a hidden contract for the way arguments are handled.
I am heve the same opinion as @AlexeyKuznetsov-DD in https://github.com/DataDog/dd-trace-java/pull/9467/files#r2325629568
While the idea of an analyzer works
The idea is to have an analyzer check this unless there is an exclusion that explains its purpose.
I wonder if a proper API wouldn't better suited there.
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.
Looks good as far as affected profiling telemetry goes.
In non-error scenarios, the behavior is fully explicit, as reporting requires the SEND_TELEMETRY marker. However, reporting errors is implicit by design. The goal is to report all errors to telemetry unless they are explicitly excluded using the EXCLUDE_TELEMETRY marker. At the same time, errors also need to be logged. Having two separate calls—one for logging and another for sending telemetry—feels redundant. Therefore, if we introduce a dedicated API, it should log errors as well to avoid duplicating calls to the logger. Consequently, the API would also need to accept the same parameters for logging. Additionally, it would be easy to forget to call the API and only use the logger. This would result in the error never being sent to telemetry. |
|
What Does This Do
#9459 follow up
Motivation
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]