Adding is_trace_root tag for APM Stats #23302

simont1 · 2024-02-29T16:48:49Z

What does this PR do?

This PR is to allow the apm-agent to send whether or not the stats come from a root trace and add this to APM Stats. Document for context: https://docs.google.com/document/d/1Ha4MxePXqcI2sMCxMfOQTaeuP7_YkkzRpxsve55aUd4/edit?usp=sharing

Motivation

Having the is_trace_root tag can help APM features to distinguish between APM stats that come from root spans vs. non-root spans.

Additional Notes

Possible Drawbacks / Trade-offs

Adding a tag will increase the cardinality of trace metrics

Describe how to test/QA your changes

We'll own the QA for this PR

bits-bot · 2024-02-29T16:48:53Z

All committers have signed the CLA.

pr-commenter · 2024-02-29T19:40:09Z

Bloop Bleep... Dogbot Here

Regression Detector Results

Run ID: 864eeb51-4b29-42f8-ba92-310cbdd2d283
Baseline: c3e5646
Comparison: 844c8db

Performance changes are noted in the perf column of each table:

✅ = significantly better comparison variant performance
❌ = significantly worse comparison variant performance
➖ = no significant change in performance

Experiments with missing or malformed data

basic_py_check

Usually, this warning means that there is no usable optimization goal data for that experiment, which could be a result of misconfiguration.

No significant changes in experiment optimization goals

Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%

There were no significant changes in experiment optimization goals at this confidence level and effect size tolerance.

Experiments ignored for regressions

Regressions in experiments with settings containing erratic: true are ignored.

perf	experiment	goal	Δ mean %	Δ mean % CI
➖	file_to_blackhole	% cpu utilization	+3.20	[-3.41, +9.80]

Fine details of change detection per experiment

perf	experiment	goal	Δ mean %	Δ mean % CI
➖	file_to_blackhole	% cpu utilization	+3.20	[-3.41, +9.80]
➖	uds_dogstatsd_to_api_cpu	% cpu utilization	+0.53	[-0.88, +1.94]
➖	otel_to_otel_logs	ingress throughput	+0.52	[-0.12, +1.15]
➖	trace_agent_json	ingress throughput	+0.00	[-0.03, +0.04]
➖	tcp_dd_logs_filter_exclude	ingress throughput	-0.00	[-0.00, +0.00]
➖	uds_dogstatsd_to_api	ingress throughput	-0.00	[-0.00, +0.00]
➖	trace_agent_msgpack	ingress throughput	-0.02	[-0.03, -0.01]
➖	process_agent_standard_check_with_stats	memory utilization	-0.12	[-0.16, -0.07]
➖	tcp_syslog_to_blackhole	ingress throughput	-0.12	[-0.18, -0.06]
➖	file_tree	memory utilization	-0.15	[-0.25, -0.05]
➖	process_agent_standard_check	memory utilization	-0.23	[-0.28, -0.18]
➖	process_agent_real_time_mode	memory utilization	-0.63	[-0.67, -0.58]
➖	idle	memory utilization	-0.88	[-0.93, -0.84]

Explanation

A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".

For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:

Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.
Its configuration does not mark it "erratic".

jdgumz · 2024-03-08T20:02:10Z

pkg/trace/stats/aggregation_test.go

@@ -85,7 +85,7 @@ func TestNewAggregation(t *testing.T) {
 				Meta:    map[string]string{"span.kind": "client", "peer.service": "remote-service"},
 			},
 			false,
-			Aggregation{BucketsAggregationKey: BucketsAggregationKey{Service: "a", SpanKind: "client"}},
+			Aggregation{BucketsAggregationKey: BucketsAggregationKey{Service: "a", SpanKind: "client", IsParentRoot: true}},


nit: I wouldn't expect a client span to be a parent root for the trace. Could we use a more realistic example?

We probably don't need to update all of these other test cases.

ajgajg1134

Code changes look good to me - definitely make sure you have a good testing plan here including an end to end test as part of QA process

jdgumz · 2024-03-19T22:43:26Z

pkg/trace/stats/concentrator_test.go

-		testSpan(now, 3, 0, 40, 2, "A2", "resource2", 2, nil),
-		testSpan(now, 4, 0, 300000000000, 2, "A2", "resource2", 2, nil), // 5 minutes trace
+		testSpan(now, 3, 1, 40, 2, "A2", "resource2", 2, nil),
+		testSpan(now, 4, 1, 300000000000, 2, "A2", "resource2", 2, nil), // 5 minutes trace


Can we avoid making changes to the existing cases? The choice of parent ID here was likely intentional.

If there's something about this test that we want to evaluate with the new flag, let's make a separate test case to do so.

I understand having to update the other tests where parent_id is 0 so we now expect the flag to be set to true.

Again though, let's see if we can avoid making changes to the parent IDs for the input test data.

👍 Moved root tag testing to a new test function

releasenotes/notes/apm-adding-is_trace_root-tag-for-APM-Stats-f3f4384105897d11.yaml

rtrieu

Minor suggestion, but approving.

…f3f4384105897d11.yaml Co-authored-by: Rosa Trieu <[email protected]>

jdgumz

We're just about there! I have some suggestions to incorporate and then I will ✅

pkg/trace/stats/aggregation.go

jdgumz · 2024-03-20T21:13:06Z

pkg/trace/stats/concentrator_test.go

@@ -418,7 +421,7 @@ func TestConcentratorStatsCounts(t *testing.T) {
 		testSpan(now, 9, 0, 30, 1, "A2", "resource2", 2, nil),
 		testSpan(now, 10, 0, 3600000000000, 1, "A2", "resourcefoo", 0, nil), // 1 hour trace
 		// present data, part of the third flush
-		testSpan(now, 6, 0, 24, 0, "A1", "resource2", 0, nil),
+		testSpan(now, 6, 100, 24, 0, "A1", "resource2", 0, nil),


Why is the parentID changed here?

jdgumz · 2024-03-20T21:39:40Z

pkg/trace/stats/concentrator_test.go

+	spans := []*pb.Span{
+		testSpan(now, 1, 0, 40, 10, "A1", "resource1", 0, nil),
+		testSpan(now, 1, 0, 40, 10, "A1", "resource1", 0, nil),
+		testSpan(now, 1, 100, 30, 10, "A1", "resource1", 0, nil),


This is a trace where we have 3 identical looking spans, two of which are root spans and one span is orphaned (its parent is 100 but there's no span here with that ID). It might be odd to have two root spans in a trace.

Let's set something up that is a little more of what we'd expect:

First span as is

Second span has span ID of 2 and parent ID of 1 (so it's a child of the first span)

Third span is a child of the second span and has a span ID of 3; let's also pass a map[string]string as below in order to mark it as a client span

Fourth span can be an orphaned case like the third span today

Here's the map to use for the third span:

map[string]string{"span.kind": "client"}

I would expect stats for only these spans:

The first span because it's a root, top-level span

The third span because it's a client span

The fourth span because it gets marked as a top-level (part of the logic for orphaned spans, see here:

datadog-agent/pkg/trace/traceutil/trace.go

Line 96 in cec4dfe

func ComputeTopLevel(trace pb.Trace) {

)

Only the stats for the first and fourth spans will have IsTraceRoot: true and the stats for the third span will have IsTraceRoot: false.

You may find it preferable to make your own spans instead of relying on testSpan. They should be simple enough structs.

Reworked test case

jdgumz · 2024-03-20T21:45:09Z

releasenotes/notes/apm-adding-is_trace_root-tag-for-APM-Stats-f3f4384105897d11.yaml

+---
+features:
+  - |
+    APM Stats now includes an is_trace_root field to indicate if the stats are from the root span of a trace.


Suggested change

APM Stats now includes an is_trace_root field to indicate if the stats are from the root span of a trace.

APM stats now include an is_trace_root field to indicate if the stats are from the root span of a trace.

pr-commenter · 2024-03-21T22:05:33Z

Test changes on VM

Use this command from test-infra-definitions to manually test this PR changes on a VM:

inv create-vm --pipeline-id=30592004 --os-family=ubuntu

pr-commenter · 2024-03-21T22:47:32Z

Regression Detector

Regression Detector Results

Run ID: d8f06fb4-2637-485f-b38e-3d7e120fd49b
Baseline: e6ec7d5
Comparison: 49678bd

Performance changes are noted in the perf column of each table:

✅ = significantly better comparison variant performance
❌ = significantly worse comparison variant performance
➖ = no significant change in performance

No significant changes in experiment optimization goals

Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%

There were no significant changes in experiment optimization goals at this confidence level and effect size tolerance.

Experiments ignored for regressions

Regressions in experiments with settings containing erratic: true are ignored.

perf	experiment	goal	Δ mean %	Δ mean % CI
➖	file_to_blackhole	% cpu utilization	-1.33	[-7.67, +5.01]

Fine details of change detection per experiment

perf	experiment	goal	Δ mean %	Δ mean % CI
➖	pycheck_1000_100byte_tags	% cpu utilization	+1.27	[-3.67, +6.22]
➖	idle	memory utilization	+0.72	[+0.66, +0.77]
➖	tcp_syslog_to_blackhole	ingress throughput	+0.35	[+0.26, +0.45]
➖	uds_dogstatsd_to_api_cpu	% cpu utilization	+0.28	[-2.48, +3.03]
➖	tcp_dd_logs_filter_exclude	ingress throughput	+0.03	[-0.01, +0.06]
➖	trace_agent_msgpack	ingress throughput	-0.01	[-0.02, -0.00]
➖	uds_dogstatsd_to_api	ingress throughput	-0.02	[-0.22, +0.19]
➖	trace_agent_json	ingress throughput	-0.03	[-0.06, +0.00]
➖	otel_to_otel_logs	ingress throughput	-0.05	[-0.46, +0.37]
➖	process_agent_real_time_mode	memory utilization	-0.07	[-0.11, -0.03]
➖	process_agent_standard_check_with_stats	memory utilization	-0.35	[-0.39, -0.31]
➖	process_agent_standard_check	memory utilization	-0.41	[-0.45, -0.36]
➖	file_tree	memory utilization	-0.49	[-0.59, -0.39]
➖	basic_py_check	% cpu utilization	-0.68	[-3.23, +1.87]
➖	file_to_blackhole	% cpu utilization	-1.33	[-7.67, +5.01]

Explanation

A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".

For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:

Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.
Its configuration does not mark it "erratic".

ajgajg1134 · 2024-03-22T15:36:21Z

/merge

dd-devflow · 2024-03-22T15:36:25Z

🚂 MergeQueue

Pull request added to the queue.

This build is going to start soon! (estimated merge in less than 29m)

Use /merge -c to cancel this operation!

* proto change to add is_parent_root * move field * proto and isParentRoot usage in the bucketAggKey * test fixes * test fixes * rename to isTraceRoot and refactor tests * unit tests * root_tag unit tests * change isTraceRoot from bool to enum * made root tags a separate test in concentrator_test.go * release notes * whitespace * Update releasenotes/notes/apm-adding-is_trace_root-tag-for-APM-Stats-f3f4384105897d11.yaml Co-authored-by: Rosa Trieu <[email protected]> * changes --------- Co-authored-by: Rosa Trieu <[email protected]>

proto change to add is_parent_root

02650e3

move field

844c8db

simont1 added 3 commits March 1, 2024 09:55

proto and isParentRoot usage in the bucketAggKey

f215f6d

test fixes

013b156

test fixes

aab1a27

jdgumz reviewed Mar 8, 2024

View reviewed changes

simont1 changed the title ~~is_parent_root aggregation key~~ Adding is_parent_root tag for APM Stats Mar 8, 2024

rename to isTraceRoot and refactor tests

1200d8b

simont1 changed the title ~~Adding is_parent_root tag for APM Stats~~ Adding is_trace_root tag for APM Stats Mar 11, 2024

unit tests

4f7fdd8

simont1 marked this pull request as ready for review March 18, 2024 15:06

simont1 requested a review from a team as a code owner March 18, 2024 15:06

root_tag unit tests

a462c1f

ajgajg1134 approved these changes Mar 19, 2024

View reviewed changes

jdgumz added the team/agent-apm trace-agent label Mar 19, 2024

change isTraceRoot from bool to enum

a997998

jdgumz reviewed Mar 19, 2024

View reviewed changes

simont1 added 2 commits March 20, 2024 09:39

made root tags a separate test in concentrator_test.go

45dde44

release notes

7dad82a

simont1 requested a review from a team as a code owner March 20, 2024 13:45

whitespace

542fa80

rtrieu reviewed Mar 20, 2024

View reviewed changes

releasenotes/notes/apm-adding-is_trace_root-tag-for-APM-Stats-f3f4384105897d11.yaml Outdated Show resolved Hide resolved

rtrieu approved these changes Mar 20, 2024

View reviewed changes

Update releasenotes/notes/apm-adding-is_trace_root-tag-for-APM-Stats-…

b32b08f

…f3f4384105897d11.yaml Co-authored-by: Rosa Trieu <[email protected]>

jdgumz requested changes Mar 20, 2024

View reviewed changes

changes

5cd8e8c

jdgumz approved these changes Mar 21, 2024

View reviewed changes

Merge branch 'main' into simont1/root-tag-aggregation-key

49678bd

dd-mergequeue bot merged commit c2a48f5 into main Mar 22, 2024
192 checks passed

dd-mergequeue bot deleted the simont1/root-tag-aggregation-key branch March 22, 2024 16:03

github-actions bot added this to the 7.53.0 milestone Mar 22, 2024

simont1 mentioned this pull request Apr 11, 2024

test/new-e2e/tests/apm: root tag e2e test #24618

Merged

knusbaum mentioned this pull request Aug 21, 2024

ddtrace/tracer: add IsTraceRoot to clients-side-stats DataDog/dd-trace-go#2821

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding is_trace_root tag for APM Stats #23302

Adding is_trace_root tag for APM Stats #23302

simont1 commented Feb 29, 2024 •

edited

Loading

bits-bot commented Feb 29, 2024 •

edited

Loading

pr-commenter bot commented Feb 29, 2024

Experiments ignored for regressions

Fine details of change detection per experiment

Explanation

jdgumz Mar 8, 2024

jdgumz Mar 8, 2024

ajgajg1134 left a comment

jdgumz Mar 19, 2024 •

edited

Loading

jdgumz Mar 19, 2024

simont1 Mar 20, 2024

rtrieu left a comment

jdgumz left a comment

jdgumz Mar 20, 2024

jdgumz Mar 20, 2024

jdgumz Mar 20, 2024

simont1 Mar 21, 2024

jdgumz Mar 20, 2024

simont1 Mar 21, 2024

pr-commenter bot commented Mar 21, 2024

pr-commenter bot commented Mar 21, 2024

Experiments ignored for regressions

Fine details of change detection per experiment

Explanation

ajgajg1134 commented Mar 22, 2024

dd-devflow bot commented Mar 22, 2024

	APM Stats now includes an is_trace_root field to indicate if the stats are from the root span of a trace.
	APM stats now include an is_trace_root field to indicate if the stats are from the root span of a trace.

Adding is_trace_root tag for APM Stats #23302

Adding is_trace_root tag for APM Stats #23302

Conversation

simont1 commented Feb 29, 2024 • edited Loading

What does this PR do?

Motivation

Additional Notes

Possible Drawbacks / Trade-offs

Describe how to test/QA your changes

bits-bot commented Feb 29, 2024 • edited Loading

pr-commenter bot commented Feb 29, 2024

Bloop Bleep... Dogbot Here

Regression Detector Results

Experiments with missing or malformed data

No significant changes in experiment optimization goals

Experiments ignored for regressions

Fine details of change detection per experiment

Explanation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajgajg1134 left a comment

Choose a reason for hiding this comment

jdgumz Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtrieu left a comment

Choose a reason for hiding this comment

jdgumz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pr-commenter bot commented Mar 21, 2024

Test changes on VM

pr-commenter bot commented Mar 21, 2024

Regression Detector

Regression Detector Results

No significant changes in experiment optimization goals

Experiments ignored for regressions

Fine details of change detection per experiment

Explanation

ajgajg1134 commented Mar 22, 2024

dd-devflow bot commented Mar 22, 2024

simont1 commented Feb 29, 2024 •

edited

Loading

bits-bot commented Feb 29, 2024 •

edited

Loading

jdgumz Mar 19, 2024 •

edited

Loading