Fix Protobuf schema sampling logic #7197

piochelepiotr · 2024-06-14T16:36:38Z

What Does This Do

When canSample returns false, trySample was not called, so the weight was not incremented.
This leads to wrong weights being calculated.
This PR updates the code to increment the weight in canSample, that should be called exactly once.

Motivation

Additional Notes

Jira ticket: [PROJ-IDENT]

piochelepiotr · 2024-06-14T16:37:24Z

...rc/main/java/datadog/trace/instrumentation/protobuf_java/AbstractMessageInstrumentation.java

@@ -36,7 +36,7 @@ public String hierarchyMarkerType() {
  public ElementMatcher<TypeDescription> hierarchyMatcher() {
    return declaresMethod(named("writeTo"))
        .and(extendsClass(named(hierarchyMarkerType())))
-        .and(not(nameStartsWith("com.google.protobuf")));
+        .and(not(nameStartsWith("com.google.protobuf")).or(named("com.google.protobuf.DynamicMessage")));


This is another change, also related to schema tracking, to track dynamic message writes. I can remove it from this PR if needed.

This is fine to me. You can add it to the additional notes section for later, if someone wonders why it was added.

pr-commenter · 2024-06-14T17:32:10Z

Benchmarks

Startup

Parameters

	Baseline	Candidate
baseline_or_candidate	baseline	candidate
git_branch	master	piotr-wolski/fix-schema-sampling
git_commit_date	1718896272	1718896254
git_commit_sha	`9bf5bbd`	`2cd7c6b`
release_version	1.36.0-SNAPSHOT~9bf5bbd556	1.36.0-SNAPSHOT~2cd7c6b057

See matching parameters

	Baseline	Candidate
application	insecure-bank	insecure-bank
ci_job_date	1718901051	1718901051
ci_job_id	549204765	549204765
ci_pipeline_id	37192287	37192287
cpu_model	Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz	Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz
module	Agent	Agent
parent	None	None
variant	iast	iast

Summary

Found 0 performance improvements and 0 performance regressions! Performance is the same for 52 metrics, 11 unstable metrics.

Startup time reports for insecure-bank

gantt
    title insecure-bank - global startup overhead: candidate=1.36.0-SNAPSHOT~2cd7c6b057, baseline=1.36.0-SNAPSHOT~9bf5bbd556

    dateFormat X
    axisFormat %s
section tracing
Agent [baseline] (1.069 s) : 0, 1068741
Total [baseline] (8.551 s) : 0, 8550612
Agent [candidate] (1.073 s) : 0, 1072506
Total [candidate] (8.61 s) : 0, 8609999
section iast
Agent [baseline] (1.176 s) : 0, 1175723
Total [baseline] (9.009 s) : 0, 9009469
Agent [candidate] (1.176 s) : 0, 1175916
Total [candidate] (8.991 s) : 0, 8990780
section iast_HARDCODED_SECRET_DISABLED
Agent [baseline] (1.169 s) : 0, 1169192
Total [baseline] (8.994 s) : 0, 8993834
Agent [candidate] (1.171 s) : 0, 1170975
Total [candidate] (8.978 s) : 0, 8978384
section iast_TELEMETRY_OFF
Agent [baseline] (1.164 s) : 0, 1163626
Total [baseline] (8.997 s) : 0, 8997220
Agent [candidate] (1.166 s) : 0, 1166375
Total [candidate] (8.982 s) : 0, 8981944

baseline results

Module	Variant	Duration	Δ tracing
Agent	tracing	1.069 s	-
Agent	iast	1.176 s	106.982 ms (10.0%)
Agent	iast_HARDCODED_SECRET_DISABLED	1.169 s	100.45 ms (9.4%)
Agent	iast_TELEMETRY_OFF	1.164 s	94.885 ms (8.9%)
Total	tracing	8.551 s	-
Total	iast	9.009 s	458.857 ms (5.4%)
Total	iast_HARDCODED_SECRET_DISABLED	8.994 s	443.222 ms (5.2%)
Total	iast_TELEMETRY_OFF	8.997 s	446.608 ms (5.2%)

candidate results

Module	Variant	Duration	Δ tracing
Agent	tracing	1.073 s	-
Agent	iast	1.176 s	103.409 ms (9.6%)
Agent	iast_HARDCODED_SECRET_DISABLED	1.171 s	98.469 ms (9.2%)
Agent	iast_TELEMETRY_OFF	1.166 s	93.869 ms (8.8%)
Total	tracing	8.61 s	-
Total	iast	8.991 s	380.781 ms (4.4%)
Total	iast_HARDCODED_SECRET_DISABLED	8.978 s	368.385 ms (4.3%)
Total	iast_TELEMETRY_OFF	8.982 s	371.945 ms (4.3%)

gantt
    title insecure-bank - break down per module: candidate=1.36.0-SNAPSHOT~2cd7c6b057, baseline=1.36.0-SNAPSHOT~9bf5bbd556

    dateFormat X
    axisFormat %s
section tracing
BytebuddyAgent [baseline] (670.321 ms) : 0, 670321
BytebuddyAgent [candidate] (672.083 ms) : 0, 672083
GlobalTracer [baseline] (305.224 ms) : 0, 305224
GlobalTracer [candidate] (306.799 ms) : 0, 306799
AppSec [baseline] (50.26 ms) : 0, 50260
AppSec [candidate] (50.581 ms) : 0, 50581
Remote Config [baseline] (686.372 µs) : 0, 686
Remote Config [candidate] (696.438 µs) : 0, 696
Telemetry [baseline] (7.585 ms) : 0, 7585
Telemetry [candidate] (7.662 ms) : 0, 7662
section iast
BytebuddyAgent [baseline] (784.629 ms) : 0, 784629
BytebuddyAgent [candidate] (785.172 ms) : 0, 785172
GlobalTracer [baseline] (294.439 ms) : 0, 294439
GlobalTracer [candidate] (293.602 ms) : 0, 293602
AppSec [baseline] (47.148 ms) : 0, 47148
AppSec [candidate] (47.16 ms) : 0, 47160
IAST [baseline] (28.54 ms) : 0, 28540
IAST [candidate] (28.968 ms) : 0, 28968
Remote Config [baseline] (620.041 µs) : 0, 620
Remote Config [candidate] (672.151 µs) : 0, 672
Telemetry [baseline] (6.984 ms) : 0, 6984
Telemetry [candidate] (6.94 ms) : 0, 6940
section iast_HARDCODED_SECRET_DISABLED
BytebuddyAgent [baseline] (779.155 ms) : 0, 779155
BytebuddyAgent [candidate] (780.49 ms) : 0, 780490
GlobalTracer [baseline] (293.342 ms) : 0, 293342
GlobalTracer [candidate] (294.346 ms) : 0, 294346
AppSec [baseline] (47.275 ms) : 0, 47275
AppSec [candidate] (47.058 ms) : 0, 47058
IAST [baseline] (28.53 ms) : 0, 28530
IAST [candidate] (28.266 ms) : 0, 28266
Remote Config [baseline] (621.565 µs) : 0, 622
Remote Config [candidate] (582.312 µs) : 0, 582
Telemetry [baseline] (6.948 ms) : 0, 6948
Telemetry [candidate] (6.872 ms) : 0, 6872
section iast_TELEMETRY_OFF
BytebuddyAgent [baseline] (775.686 ms) : 0, 775686
BytebuddyAgent [candidate] (777.03 ms) : 0, 777030
GlobalTracer [baseline] (292.924 ms) : 0, 292924
GlobalTracer [candidate] (293.435 ms) : 0, 293435
AppSec [baseline] (47.12 ms) : 0, 47120
AppSec [candidate] (47.252 ms) : 0, 47252
IAST [baseline] (24.728 ms) : 0, 24728
IAST [candidate] (27.839 ms) : 0, 27839
Remote Config [baseline] (606.097 µs) : 0, 606
Remote Config [candidate] (587.767 µs) : 0, 588
Telemetry [baseline] (9.227 ms) : 0, 9227
Telemetry [candidate] (6.892 ms) : 0, 6892

Startup time reports for petclinic

gantt
    title petclinic - global startup overhead: candidate=1.36.0-SNAPSHOT~2cd7c6b057, baseline=1.36.0-SNAPSHOT~9bf5bbd556

    dateFormat X
    axisFormat %s
section tracing
Agent [baseline] (1.063 s) : 0, 1062835
Total [baseline] (10.331 s) : 0, 10331181
Agent [candidate] (1.065 s) : 0, 1064993
Total [candidate] (10.304 s) : 0, 10304067
section appsec
Agent [baseline] (1.183 s) : 0, 1183246
Total [baseline] (10.499 s) : 0, 10499364
Agent [candidate] (1.181 s) : 0, 1181240
Total [candidate] (10.521 s) : 0, 10521356
section iast
Agent [baseline] (1.18 s) : 0, 1179931
Total [baseline] (10.911 s) : 0, 10910787
Agent [candidate] (1.169 s) : 0, 1169022
Total [candidate] (10.754 s) : 0, 10753865
section profiling
Agent [baseline] (1.26 s) : 0, 1260311
Total [baseline] (10.601 s) : 0, 10601467
Agent [candidate] (1.263 s) : 0, 1263348
Total [candidate] (10.595 s) : 0, 10594991

baseline results

Module	Variant	Duration	Δ tracing
Agent	tracing	1.063 s	-
Agent	appsec	1.183 s	120.411 ms (11.3%)
Agent	iast	1.18 s	117.097 ms (11.0%)
Agent	profiling	1.26 s	197.476 ms (18.6%)
Total	tracing	10.331 s	-
Total	appsec	10.499 s	168.183 ms (1.6%)
Total	iast	10.911 s	579.606 ms (5.6%)
Total	profiling	10.601 s	270.285 ms (2.6%)

candidate results

Module	Variant	Duration	Δ tracing
Agent	tracing	1.065 s	-
Agent	appsec	1.181 s	116.247 ms (10.9%)
Agent	iast	1.169 s	104.03 ms (9.8%)
Agent	profiling	1.263 s	198.356 ms (18.6%)
Total	tracing	10.304 s	-
Total	appsec	10.521 s	217.289 ms (2.1%)
Total	iast	10.754 s	449.797 ms (4.4%)
Total	profiling	10.595 s	290.924 ms (2.8%)

gantt
    title petclinic - break down per module: candidate=1.36.0-SNAPSHOT~2cd7c6b057, baseline=1.36.0-SNAPSHOT~9bf5bbd556

    dateFormat X
    axisFormat %s
section tracing
BytebuddyAgent [baseline] (665.618 ms) : 0, 665618
BytebuddyAgent [candidate] (667.327 ms) : 0, 667327
GlobalTracer [baseline] (304.156 ms) : 0, 304156
GlobalTracer [candidate] (304.461 ms) : 0, 304461
AppSec [baseline] (50.308 ms) : 0, 50308
AppSec [candidate] (50.437 ms) : 0, 50437
Remote Config [baseline] (687.582 µs) : 0, 688
Remote Config [candidate] (680.729 µs) : 0, 681
Telemetry [baseline] (7.568 ms) : 0, 7568
Telemetry [candidate] (7.52 ms) : 0, 7520
section appsec
BytebuddyAgent [baseline] (676.677 ms) : 0, 676677
BytebuddyAgent [candidate] (675.938 ms) : 0, 675938
GlobalTracer [baseline] (297.747 ms) : 0, 297747
GlobalTracer [candidate] (297.454 ms) : 0, 297454
AppSec [baseline] (153.599 ms) : 0, 153599
AppSec [candidate] (153.705 ms) : 0, 153705
IAST [baseline] (22.174 ms) : 0, 22174
IAST [candidate] (21.107 ms) : 0, 21107
Remote Config [baseline] (640.798 µs) : 0, 641
Remote Config [candidate] (634.547 µs) : 0, 635
Telemetry [baseline] (9.056 ms) : 0, 9056
Telemetry [candidate] (8.495 ms) : 0, 8495
section iast
BytebuddyAgent [baseline] (782.665 ms) : 0, 782665
BytebuddyAgent [candidate] (778.929 ms) : 0, 778929
GlobalTracer [baseline] (298.513 ms) : 0, 298513
GlobalTracer [candidate] (293.512 ms) : 0, 293512
AppSec [baseline] (48.19 ms) : 0, 48190
AppSec [candidate] (47.226 ms) : 0, 47226
IAST [baseline] (28.75 ms) : 0, 28750
IAST [candidate] (28.316 ms) : 0, 28316
Remote Config [baseline] (654.255 µs) : 0, 654
Remote Config [candidate] (769.233 µs) : 0, 769
Telemetry [baseline] (7.834 ms) : 0, 7834
Telemetry [candidate] (6.98 ms) : 0, 6980
section profiling
BytebuddyAgent [baseline] (661.606 ms) : 0, 661606
BytebuddyAgent [candidate] (663.499 ms) : 0, 663499
GlobalTracer [baseline] (385.043 ms) : 0, 385043
GlobalTracer [candidate] (386.763 ms) : 0, 386763
AppSec [baseline] (51.707 ms) : 0, 51707
AppSec [candidate] (51.689 ms) : 0, 51689
Remote Config [baseline] (728.046 µs) : 0, 728
Remote Config [candidate] (737.417 µs) : 0, 737
Telemetry [baseline] (7.351 ms) : 0, 7351
Telemetry [candidate] (7.436 ms) : 0, 7436
ProfilingAgent [baseline] (97.019 ms) : 0, 97019
ProfilingAgent [candidate] (96.189 ms) : 0, 96189
Profiling [baseline] (97.045 ms) : 0, 97045
Profiling [candidate] (96.214 ms) : 0, 96214

Load

Parameters

	Baseline	Candidate
baseline_or_candidate	baseline	candidate
end_time	2024-06-20T15:59:58	2024-06-20T16:06:48
git_branch	master	piotr-wolski/fix-schema-sampling
git_commit_date	1718896272	1718896254
git_commit_sha	`9bf5bbd`	`2cd7c6b`
release_version	1.36.0-SNAPSHOT~9bf5bbd556	1.36.0-SNAPSHOT~2cd7c6b057
start_time	2024-06-20T15:59:44	2024-06-20T16:06:35

See matching parameters

	Baseline	Candidate
application	insecure-bank	insecure-bank
ci_job_date	1718899956	1718899956
ci_job_id	549204766	549204766
ci_pipeline_id	37192287	37192287
cpu_model	Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz	Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz
variant	iast	iast

Summary

Found 0 performance improvements and 0 performance regressions! Performance is the same for 11 metrics, 17 unstable metrics.

Request duration reports for petclinic

gantt
    title petclinic - request duration [CI 0.99] : candidate=1.36.0-SNAPSHOT~2cd7c6b057, baseline=1.36.0-SNAPSHOT~9bf5bbd556
    dateFormat X
    axisFormat %s
section baseline
no_agent (1.351 ms) : 1332, 1371
.   : milestone, 1351,
appsec (1.722 ms) : 1698, 1746
.   : milestone, 1722,
appsec_no_iast (1.741 ms) : 1717, 1765
.   : milestone, 1741,
iast (1.501 ms) : 1479, 1523
.   : milestone, 1501,
profiling (1.479 ms) : 1454, 1503
.   : milestone, 1479,
tracing (1.461 ms) : 1437, 1486
.   : milestone, 1461,
section candidate
no_agent (1.351 ms) : 1332, 1370
.   : milestone, 1351,
appsec (1.716 ms) : 1691, 1741
.   : milestone, 1716,
appsec_no_iast (1.707 ms) : 1682, 1732
.   : milestone, 1707,
iast (1.482 ms) : 1460, 1505
.   : milestone, 1482,
profiling (1.485 ms) : 1460, 1509
.   : milestone, 1485,
tracing (1.482 ms) : 1458, 1506
.   : milestone, 1482,

baseline results

Variant	Request duration [CI 0.99]	Δ no_agent
no_agent	1.351 ms [1.332 ms, 1.371 ms]	-
appsec	1.722 ms [1.698 ms, 1.746 ms]	370.666 µs (27.4%)
appsec_no_iast	1.741 ms [1.717 ms, 1.765 ms]	389.695 µs (28.8%)
iast	1.501 ms [1.479 ms, 1.523 ms]	149.857 µs (11.1%)
profiling	1.479 ms [1.454 ms, 1.503 ms]	127.486 µs (9.4%)
tracing	1.461 ms [1.437 ms, 1.486 ms]	110.04 µs (8.1%)

candidate results

Variant	Request duration [CI 0.99]	Δ no_agent
no_agent	1.351 ms [1.332 ms, 1.37 ms]	-
appsec	1.716 ms [1.691 ms, 1.741 ms]	365.639 µs (27.1%)
appsec_no_iast	1.707 ms [1.682 ms, 1.732 ms]	355.859 µs (26.3%)
iast	1.482 ms [1.46 ms, 1.505 ms]	131.497 µs (9.7%)
profiling	1.485 ms [1.46 ms, 1.509 ms]	133.755 µs (9.9%)
tracing	1.482 ms [1.458 ms, 1.506 ms]	131.526 µs (9.7%)

Request duration reports for insecure-bank

gantt
    title insecure-bank - request duration [CI 0.99] : candidate=1.36.0-SNAPSHOT~2cd7c6b057, baseline=1.36.0-SNAPSHOT~9bf5bbd556
    dateFormat X
    axisFormat %s
section baseline
no_agent (364.482 µs) : 344, 385
.   : milestone, 364,
iast (486.278 µs) : 465, 508
.   : milestone, 486,
iast_FULL (556.843 µs) : 536, 578
.   : milestone, 557,
iast_GLOBAL (509.407 µs) : 488, 531
.   : milestone, 509,
iast_HARDCODED_SECRET_DISABLED (485.634 µs) : 464, 507
.   : milestone, 486,
iast_INACTIVE (458.3 µs) : 437, 480
.   : milestone, 458,
iast_TELEMETRY_OFF (473.113 µs) : 452, 495
.   : milestone, 473,
tracing (447.078 µs) : 425, 469
.   : milestone, 447,
section candidate
no_agent (363.759 µs) : 344, 383
.   : milestone, 364,
iast (483.794 µs) : 463, 505
.   : milestone, 484,
iast_FULL (549.832 µs) : 529, 571
.   : milestone, 550,
iast_GLOBAL (523.189 µs) : 500, 546
.   : milestone, 523,
iast_HARDCODED_SECRET_DISABLED (494.246 µs) : 473, 515
.   : milestone, 494,
iast_INACTIVE (457.538 µs) : 436, 479
.   : milestone, 458,
iast_TELEMETRY_OFF (478.663 µs) : 457, 500
.   : milestone, 479,
tracing (447.027 µs) : 426, 468
.   : milestone, 447,

baseline results

Variant	Request duration [CI 0.99]	Δ no_agent
no_agent	364.482 µs [344.437 µs, 384.526 µs]	-
iast	486.278 µs [464.913 µs, 507.642 µs]	121.796 µs (33.4%)
iast_FULL	556.843 µs [535.614 µs, 578.071 µs]	192.361 µs (52.8%)
iast_GLOBAL	509.407 µs [487.905 µs, 530.909 µs]	144.925 µs (39.8%)
iast_HARDCODED_SECRET_DISABLED	485.634 µs [464.48 µs, 506.789 µs]	121.153 µs (33.2%)
iast_INACTIVE	458.3 µs [436.722 µs, 479.877 µs]	93.818 µs (25.7%)
iast_TELEMETRY_OFF	473.113 µs [451.686 µs, 494.539 µs]	108.631 µs (29.8%)
tracing	447.078 µs [425.323 µs, 468.834 µs]	82.597 µs (22.7%)

candidate results

Variant	Request duration [CI 0.99]	Δ no_agent
no_agent	363.759 µs [344.374 µs, 383.143 µs]	-
iast	483.794 µs [462.779 µs, 504.809 µs]	120.036 µs (33.0%)
iast_FULL	549.832 µs [528.755 µs, 570.91 µs]	186.073 µs (51.2%)
iast_GLOBAL	523.189 µs [499.933 µs, 546.444 µs]	159.43 µs (43.8%)
iast_HARDCODED_SECRET_DISABLED	494.246 µs [473.373 µs, 515.119 µs]	130.487 µs (35.9%)
iast_INACTIVE	457.538 µs [436.234 µs, 478.843 µs]	93.78 µs (25.8%)
iast_TELEMETRY_OFF	478.663 µs [456.894 µs, 500.433 µs]	114.904 µs (31.6%)
tracing	447.027 µs [426.023 µs, 468.032 µs]	83.269 µs (22.9%)

Dacapo

PerfectSlayer

LTGM

...rc/main/java/datadog/trace/instrumentation/protobuf_java/AbstractMessageInstrumentation.java

vandonr · 2024-06-20T14:26:05Z

...-core/src/test/groovy/datadog/trace/core/datastreams/DefaultDataStreamsMonitoringTest.groovy

+    dataStreams.canSampleSchema("schema1")
    dataStreams.trySampleSchema("schema1") == 1
+    dataStreams.canSampleSchema("schema2")
    dataStreams.trySampleSchema("schema2") == 1
-    dataStreams.trySampleSchema("schema1") == 0
-    dataStreams.trySampleSchema("schema1") == 0
+    !dataStreams.canSampleSchema("schema1")
+    !dataStreams.canSampleSchema("schema1")
    timeSource.advance(30*1e9 as long)
+    dataStreams.canSampleSchema("schema1")
    dataStreams.trySampleSchema("schema1") == 3


I think a future maintainer would be grateful if there was one or two comments explaining why we expect those results

vandonr

lessgo

piochelepiotr requested review from a team as code owners June 14, 2024 16:36

piochelepiotr requested review from PerfectSlayer and ygree June 14, 2024 16:36

piochelepiotr commented Jun 14, 2024

View reviewed changes

piochelepiotr changed the title ~~fix schema sampling logic~~ bug fix: schema sampling logic Jun 14, 2024

fix schema sampling logic

f1747e1

piochelepiotr force-pushed the piotr-wolski/fix-schema-sampling branch from 9d5f3a9 to f1747e1 Compare June 14, 2024 16:45

kr-igor approved these changes Jun 14, 2024

View reviewed changes

fix test

15ae32f

piochelepiotr force-pushed the piotr-wolski/fix-schema-sampling branch from bd0b276 to 15ae32f Compare June 14, 2024 20:12

PerfectSlayer approved these changes Jun 17, 2024

View reviewed changes

PerfectSlayer changed the title ~~bug fix: schema sampling logic~~ Fix Protobuf schema sampling logic Jun 17, 2024

PerfectSlayer added type: bug inst: others All other instrumentations labels Jun 17, 2024

piochelepiotr added 2 commits June 17, 2024 14:59

update Avro schema sampling

f3a65d6

Merge branch 'master' into piotr-wolski/fix-schema-sampling

0893fe6

vandonr reviewed Jun 20, 2024

View reviewed changes

add comments to test

2cd7c6b

vandonr approved these changes Jun 20, 2024

View reviewed changes

piochelepiotr merged commit 174ea28 into master Jun 20, 2024
72 of 84 checks passed

piochelepiotr deleted the piotr-wolski/fix-schema-sampling branch June 20, 2024 22:28

github-actions bot added this to the 1.36.0 milestone Jun 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Protobuf schema sampling logic #7197

Fix Protobuf schema sampling logic #7197

piochelepiotr commented Jun 14, 2024

piochelepiotr Jun 14, 2024 •

edited

Loading

PerfectSlayer Jun 17, 2024

pr-commenter bot commented Jun 14, 2024 •

edited

Loading

PerfectSlayer left a comment

vandonr Jun 20, 2024

vandonr left a comment

Fix Protobuf schema sampling logic #7197

Fix Protobuf schema sampling logic #7197

Conversation

piochelepiotr commented Jun 14, 2024

What Does This Do

Motivation

Additional Notes

piochelepiotr Jun 14, 2024 • edited Loading

Choose a reason for hiding this comment

PerfectSlayer Jun 17, 2024

Choose a reason for hiding this comment

pr-commenter bot commented Jun 14, 2024 • edited Loading

Benchmarks

Startup

Parameters

Summary

Load

Parameters

Summary

Dacapo

PerfectSlayer left a comment

Choose a reason for hiding this comment

vandonr Jun 20, 2024

Choose a reason for hiding this comment

vandonr left a comment

Choose a reason for hiding this comment

piochelepiotr Jun 14, 2024 •

edited

Loading

pr-commenter bot commented Jun 14, 2024 •

edited

Loading