[Tracer] Stats computation: Drop P0 traces by zacharycmontoya · Pull Request #3048 · DataDog/dd-trace-dotnet

zacharycmontoya · 2022-08-03T22:29:41Z

Summary of changes

When stats computation is enabled and the tracer can drop P0 traces (assumed to be true, will check with agent in followup PR), P0 traces are not serialized and the "Datadog-Client-Dropped-P0-Traces" and "Datadog-Client-Dropped-P0-Spans" request headers are set on the next trace payload.

Based on top of #3047, this is PR 2/3 in an attempt to break down the massive PR #2988

Reason for change

Dropping P0 traces is the biggest advantage of enabling stats computation in the tracer, because a large amount of span serialization (by the tracer) and deserialization (by the trace agent) is removed entirely.

Implementation details

TraceContext changes

Background: The TraceContext object is responsible for flushing finished spans to its Tracer object reference when either all spans in the local trace are finished or the number of finished spans in the ongoing trace exceeds the partial flush limit. After this point, the finished spans are sent to the AgentWriter to be buffered/serialized, so we must make the decision here whether to drop P0 spans.

If Tracer.CanDropP0s == true, then the following algorithm is run on each of the finished spans to determine if the span should be kept. If at least one span is kept, then the entire trace is kept. If not, then the entire trace is dropped.

Is the sampling priority greater than 0? If so, keep it.
Does the span have an error? If so, keep it.
- The agent is more advanced and applies a sampling rate sharded by stats key, but that's not necessary right now.
Does the span have an app analytics sample rate? If so, does the sampling decision determine it should be sampled?
If the span generates a stat point (must be top-level/measured), is the stat point unique? If so, keep the span.

We then pass the keep/drop decision along with the finished span array to the AgentWriter.WriteTrace API.

Note: There will need to be some special handling if a trace is supposed to be sampled out but the new single span ingestion controls determine that single spans should be kept. This is documented in the RFC and will need to be handled when the feature is implemented in the .NET Tracer.

AgentWriter / IApi changes

Background: The AgentWriter puts finished spans into buffers, and when a buffer is full those spans are sent to the trace agent by calling IApi.SendTracesAsync.

When the AgentWriter receives spans, it now accumulates the number of dropped P0 traces and dropped P0 spans. Then, when a span buffer needs to be serialized and IApi.SendTracesAsync is invoked, it passes along these counts and they are added as headers to the trace agent request.

Test coverage

Adds unit tests in TraceContextTests and integration tests in StatsTests to assert that P0 traces are dropped and the headers are sent correctly.

Other details

This PR also adds a configuration key _DD_TRACE_STATS_COMPUTATION_INTERVAL to configure the interval for sending stats. This should be considered a non-public setting and could change at any time.

Remaining Work Items

Refactor the decision to sample spans
Fix the ErrorSampler as noted here. The refactoring should make this implementation simpler
Spans that are sampled by the RareSampler must have the _dd.rare metric, as noted here. The other complication is that the RareSampler only runs if a span was not kept by the PrioritySampler (SamplingPriority > 0). The refactoring should make this implementation simpler

pierotibou

A few general comments:

It's a bit late for that, but I wonder if we shouldn't have given the AgentWriter the responsability to aggregate stats. We could maybe have put all stats related code in a processor called in the TracerManager. This would have allowed us to handle in one place both dropping or not traces and aggregating stats. If I'm right we could have also moved all the extra logic in TraceContextthere. If what I'm suggesting makes sense, we could add this for a future refacto in the backlog. The only downside I see to the approach I'm suggesting is that we would need to pass the extra info that goes in headers down the pipeline, but that doesn't a big deal.
Talking about processors, I don't think we have plugged the obfuscation processors yet, have we?
Also telemetry :)

pierotibou · 2022-08-04T08:03:42Z


            ArraySegment<Span> spansToWrite = default;
-
+            bool shouldKeepSpan = true;


Nit - ANd feel free to ignore me as I'm bad at this, but I would have named it shouldSendSpan because this is ultimately what we do

Yeah I wasn't 100% sure myself whether to use the word "keep" or "send". We tend to have the keep/drop paradigm for sampling decisions, so I chose to use "keep" as much as possible

pierotibou · 2022-08-04T08:24:46Z

+
+            if (span.GetMetric(Trace.Tags.Analytics) is double rate)
+            {
+                return ((span.TraceId * KnuthFactor) % TracerConstants.MaxTraceId) <= (rate * TracerConstants.MaxTraceId);


Not a fan of replicating this logic here, but we can rework it later, maybe in the SingleSpanIngestion OKR for instance

pierotibou · 2022-08-04T09:13:08Z

-
        private static readonly IDatadogLogger Log = DatadogLogging.GetLoggerFor<StatsAggregator>();

+        private readonly HashSet<StatsAggregationKey> _keys;


I have a hard time estimating the cardinality of a key. I assume it's driven mainly by resource right? We shouldn't have that many of them hopefully. I assume we don't but would we have this data somewhere?

Yeah, this naïve implementation could start to build up without a way to limit the set. I think you're right that it's mainly driven by the cardinality of resource names. Here's how a key is composed, which is a unique combination of the following fields:

Resource: Most likely to be high-cardinality

Service: Low cardinality

OperationName: Low cardinality?

Type: Low cardinality?

HttpStatusCode: Low cardinality. It's only expected on type http (0 otherwise) and I expect the range of status codes to be limited

IsSyntheticsRequest: Low cardinality. true/false

Yep so resource is the one with the high cardinality (plus I suspect that will be the longest string as well). So the main factor for the size of this set will be resources, no?

Maybe there's a metric the stats team emit to follow the number of resources, that would give us an idea. We could ask on #apm-stats just in case.
And to be clear, without more data, I'm not challenging the implementation (best is the enemy of good)

Just as a reminder, there's an ongoing issue with high cardinality of resource names for outgoing http requests that contain short identifier segments. There could be thousands of them 😬

zacharycmontoya · 2022-08-04T18:16:29Z

It's a bit late for that, but I wonder if we shouldn't have given the AgentWriter the responsability to aggregate stats.

I think you're right and that we can improve this with refactoring

Talking about processors, I don't think we have plugged the obfuscation processors yet, have we?

Dangit, they're not plugged in. So, if the processors were initialized, they would be called correctly because the sequence of calls goes like this: TraceContext.CloseSpan() => Tracer.Write() => TracerManager.WriteTrace() => TraceProcessors.foreach(p => p.Process)

However, I don't see them being initialized yet in our TracerManager class. I do see them being configured in our CITracerManager class so that's a good reference. I created a JIRA to track this.

Also telemetry :)

🤦🏼 I created a JIRA to track this.

bouwkast · 2022-08-04T19:46:20Z

What exactly is a P0 trace? 😃

zacharycmontoya · 2022-08-05T02:13:38Z

I've added processors in #3054

zacharycmontoya · 2022-08-05T02:25:38Z

What exactly is a P0 trace? 😃

Good question! For background, since applications can generate huge amounts of traces, it could be cost-prohibitive to send all spans to the backend. As a result, we traditionally would send all of the application's spans to the Datadog Agent, and the Datadog Agent would look at a number of factors to determine which traces should be forwarded to the backend and which could be dropped.

One of the inputs into the decision-making is a field called SamplingPriority, whose values you can find here. This can be configured both by the user and by us as we're processing the spans. Notably, values <= 0 should be rejected (except for some special cases), so internally our RFC's and docs may refer to spans with a <= 0 sampling priority as a P0 trace/span. This is also a relatively new term for me that I picked up in the last two months, so I hope I'm using it correctly 😆

pierotibou · 2022-08-05T12:11:30Z

What exactly is a P0 trace? 😃

Great question :) And What Zach said. If you want to learn more, this not yet officialy doc, should be your go to
https://datadoghq.atlassian.net/wiki/spaces/APM/pages/2564915820/Trace+Ingestion+Mechanisms#Terminology

andrewlock

LGTM, mostly nits, though I wonder if we can refactor where the calculations happen, as you've already discussed

andrewlock · 2022-08-08T09:37:24Z

+            var state = new SendTracesState(traces, numberOfTraces, statsComputationEnabled, numberOfDroppedP0Traces, numberOfDroppedP0Spans);

            return SendWithRetry(_tracesEndpoint, _sendTraces, state);


nit (not related to this PR per-se), but as SendStatsState and SendTracesState are readonly struct, the state parameter in SendWithRetry() should be marked in to remove defensive copies. Same goes for child functions these are passed to, especially as SendTracesState is getting bigger

That's a good point. Is it okay if we defer this topic for a follow up PR?

andrewlock · 2022-08-08T09:42:07Z

        /// </summary>
        internal StatsBuffer CurrentBuffer => _buffers[_currentBuffer];

+        public bool? CanComputeStats { get; private set; } = true;


Just noting that as per the PR description this is set to null initially and updated accordingly based on agent support in the follow up PR #3049

andrewlock · 2022-08-08T09:43:49Z

            if ((!span.IsTopLevel && span.GetMetric(Tags.Measured) != 1.0) || span.GetMetric(Tags.PartialSnapshot) > 0)
            {
-                return;
+                return false;
            }


This could still be a "rare" key couldn't it? It could also have an error....

Looking at the RareSampler, the sampler only runs on top-level/measured spans, so we're consistent there.

As for the error part, I think you've identified a bug in the Java implementation. That's the reference implementation I followed, where errors are only checked on top-level/measured spans. However, the agent checks for errors on any span in the trace chunk. I will revise this.

Thinking on this one particular issue more, the bug you identified was that we could return false even when there's an error. However, I don't think this is actually an issue. This return value is only used as a fallback after the TraceContext ran its rules. When the TraceContext closed the trace and sent it to serialization, we would have already detected that there was at least one error within the trace so we would have called AgentWriter.SerializeTrace(ArraySegment trace, bool shouldSerializeSpans) with the value true which would disregard the value returned here and send the trace to the trace agent. So, this is only a fallback and the initial decision resulted in sending the error-containing trace.

This will go away soon with the new refactor, but I thought it was worth reporting

andrewlock · 2022-08-08T09:45:21Z

        }

-        public void WriteTrace(ArraySegment<Span> trace)
+        public void WriteTrace(ArraySegment<Span> trace, bool shouldSerializeSpans)


I guess there's a question of how this works with CI visibility, but they seem somewhat orthogonal, so I don't see an issue with this going unused here

For now, this is a consequence of where I inserted the decision-making in the TraceContext. With some refactoring, we may be able to move the decision-making inside the WriteTrace method so we don't have this unused paramater

andrewlock · 2022-08-08T10:19:54Z

+        // - Is the current sampling priority > 0? Return true.
+        // - Have any errors been seen? Return true
+        // - If the current span doesn't have Metrics["_dd1.sr.eausr"] (aka AnalyticsSampleRate), skip. If it does, run the Knuth sampling decision on the metric value and return the result.
+        private bool ShouldKeepSpan(Span span)


So we currently calculate whether we should keep the span here based on the rules about, then we add the extra information from the stats aggregation later too. It seems a little off to be calculating two halves of the same thing in two different places, any chance we can merge them somehow, either pushing the stats aggregation up to here, or the span inspection down? 🤔

EDIT: I see Pierre had the same thoughts 🙂

pierotibou · 2022-08-16T13:59:34Z


            var key = BuildKey(span);

+            var isNewKey = _keys.Add(key);


From the trace ingestion mechanism doc, it seems we need to add one tag in that case:

_dd.rare: Sampling rate of the RareSampler

andrewlock

LGTM! Sorry about the ArraySegment comment, may have just been unnecessary churn looking at the changes 😅 )

Caveat: We may have the StatsComputationSetting enabled but the agent we try to reach does not support it. That complicated mess will be addressed next.

…0-Traces" and "Datadog-Client-Dropped-P0-Spans" when we drop P0 traces

… stats point is completely new or the stats point contains an error, force the trace to be serialized to the trace agent.

… stats: _DD_TRACE_STATS_COMPUTATION_INTERVAL This value has an underscore prefix so this should be considered non-public. The main motivation for adding a configurable interval is for testing, as the 10-second default duration is excessively long for integration testing.

…, int offset, int count) => IStatsAggregator.AddRange(ArraySegment spans)

…is time changing the way the duration is tested. Please work

…the default value of 10. Now that we only wait once at the end, this should be acceptable

andrewlock · 2022-08-26T21:39:04Z

Benchmarks Report 🐌

Benchmarks for #3048 compared to master:

1 benchmarks are slower, with geometric mean 1.205
All benchmarks have the same allocations

The following thresholds were used for comparing the benchmark speeds:

Mann–Whitney U test with statistical test for significance of 5%
Only results indicating a difference greater than 10% and 0.3 ns are considered.

Allocation changes below 0.5% are ignored.

Benchmark details

Benchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`WriteAndFlushEnrichedTraces`	net472	716μs	471ns	1.76μs	0.355	3.18 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	467μs	338ns	1.26μs	0	2.59 KB
#3048	`WriteAndFlushEnrichedTraces`	net472	717μs	572ns	2.22μs	0.359	3.18 KB
#3048	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	462μs	239ns	924ns	0	2.58 KB

Benchmarks.Trace.AppSecBodyBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #3048

Benchmark	diff/base	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.AppSecBodyBenchmark.AllCycleSimpleBody‑net472	1.205	186.90	225.29

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`AllCycleSimpleBody`	net472	187ns	0.146ns	0.566ns	0.0676	0	425 B
master	`AllCycleSimpleBody`	netcoreapp3.1	241ns	0.332ns	1.2ns	0.0058	0	424 B
master	`AllCycleMoreComplexBody`	net472	185ns	0.144ns	0.559ns	0.0637	0	401 B
master	`AllCycleMoreComplexBody`	netcoreapp3.1	235ns	0.371ns	1.44ns	0.0054	0	400 B
master	`BodyExtractorSimpleBody`	net472	253ns	0.2ns	0.775ns	0.0574	0	361 B
master	`BodyExtractorSimpleBody`	netcoreapp3.1	230ns	0.265ns	0.99ns	0.00373	0	272 B
master	`BodyExtractorMoreComplexBody`	net472	14.5μs	19.7ns	73.7ns	1.21	0.0216	7.62 KB
master	`BodyExtractorMoreComplexBody`	netcoreapp3.1	12.3μs	17.3ns	67ns	0.0863	0	6.75 KB
#3048	`AllCycleSimpleBody`	net472	226ns	0.387ns	1.45ns	0.0675	0	425 B
#3048	`AllCycleSimpleBody`	netcoreapp3.1	244ns	0.308ns	1.19ns	0.00578	0	424 B
#3048	`AllCycleMoreComplexBody`	net472	189ns	0.299ns	1.16ns	0.0637	0	401 B
#3048	`AllCycleMoreComplexBody`	netcoreapp3.1	243ns	0.281ns	1.09ns	0.00549	0	400 B
#3048	`BodyExtractorSimpleBody`	net472	269ns	0.638ns	2.47ns	0.0573	0	361 B
#3048	`BodyExtractorSimpleBody`	netcoreapp3.1	229ns	0.503ns	1.95ns	0.00363	0	272 B
#3048	`BodyExtractorMoreComplexBody`	net472	15.1μs	28.8ns	111ns	1.21	0.0148	7.62 KB
#3048	`BodyExtractorMoreComplexBody`	netcoreapp3.1	12.7μs	22.5ns	87.2ns	0.0881	0	6.75 KB

Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
master	`SendRequest`	netcoreapp3.1	182μs	201ns	780ns	0.273	20.57 KB
#3048	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
#3048	`SendRequest`	netcoreapp3.1	181μs	156ns	603ns	0.18	20.57 KB

Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`ExecuteNonQuery`	net472	1.82μs	0.707ns	2.65ns	0.15	0.000908	947 B
master	`ExecuteNonQuery`	netcoreapp3.1	1.38μs	0.452ns	1.75ns	0.0124	0	936 B
#3048	`ExecuteNonQuery`	net472	1.87μs	0.622ns	2.33ns	0.15	0.000929	947 B
#3048	`ExecuteNonQuery`	netcoreapp3.1	1.44μs	0.665ns	2.49ns	0.0126	0	936 B

Benchmarks.Trace.ElasticsearchBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`CallElasticsearch`	net472	2.52μs	0.648ns	2.51ns	0.183	1.16 KB
master	`CallElasticsearch`	netcoreapp3.1	1.54μs	0.525ns	2.04ns	0.0147	1.1 KB
master	`CallElasticsearchAsync`	net472	2.67μs	2.83ns	11ns	0.205	1.29 KB
master	`CallElasticsearchAsync`	netcoreapp3.1	1.59μs	0.611ns	2.29ns	0.0167	1.22 KB
#3048	`CallElasticsearch`	net472	2.4μs	0.684ns	2.65ns	0.183	1.16 KB
#3048	`CallElasticsearch`	netcoreapp3.1	1.58μs	0.647ns	2.42ns	0.0151	1.1 KB
#3048	`CallElasticsearchAsync`	net472	2.49μs	2.33ns	8.71ns	0.205	1.29 KB
#3048	`CallElasticsearchAsync`	netcoreapp3.1	1.64μs	1.73ns	6.47ns	0.0167	1.22 KB

Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`ExecuteAsync`	net472	2.54μs	9.2ns	35.6ns	0.224	1.41 KB
master	`ExecuteAsync`	netcoreapp3.1	1.66μs	4.22ns	16.3ns	0.0178	1.34 KB
#3048	`ExecuteAsync`	net472	2.7μs	4.52ns	17.5ns	0.223	1.41 KB
#3048	`ExecuteAsync`	netcoreapp3.1	1.76μs	1.14ns	4.26ns	0.0175	1.34 KB

Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendAsync`	net472	5.69μs	24.4ns	94.5ns	0.439	2.77 KB
master	`SendAsync`	netcoreapp3.1	3.57μs	9.34ns	36.2ns	0.0351	2.6 KB
#3048	`SendAsync`	net472	5.73μs	9.22ns	35.7ns	0.439	2.77 KB
#3048	`SendAsync`	netcoreapp3.1	3.53μs	5.14ns	19.2ns	0.035	2.6 KB

Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	3.13μs	2.34ns	9.05ns	0.287	1.81 KB
master	`EnrichedLog`	netcoreapp3.1	2.56μs	1.49ns	5.59ns	0.0242	1.85 KB
#3048	`EnrichedLog`	net472	3.1μs	3.12ns	12.1ns	0.287	1.81 KB
#3048	`EnrichedLog`	netcoreapp3.1	2.54μs	1.84ns	6.89ns	0.0256	1.85 KB

Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`EnrichedLog`	net472	152μs	128ns	494ns	0.686	0.229	4.65 KB
master	`EnrichedLog`	netcoreapp3.1	113μs	74.4ns	268ns	0.057	0	4.49 KB
#3048	`EnrichedLog`	net472	152μs	129ns	466ns	0.689	0.23	4.65 KB
#3048	`EnrichedLog`	netcoreapp3.1	116μs	276ns	1.07μs	0.0577	0	4.49 KB

Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`EnrichedLog`	net472	5.6μs	13.9ns	53.7ns	0.567	0.00278	3.59 KB
master	`EnrichedLog`	netcoreapp3.1	4.18μs	12.2ns	47.2ns	0.0538	0	3.91 KB
#3048	`EnrichedLog`	net472	5.77μs	6.95ns	26.9ns	0.567	0.00289	3.59 KB
#3048	`EnrichedLog`	netcoreapp3.1	4.38μs	5.24ns	19.6ns	0.0518	0	3.91 KB

Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendReceive`	net472	2.19μs	1.66ns	6.44ns	0.217	1.37 KB
master	`SendReceive`	netcoreapp3.1	1.77μs	0.673ns	2.52ns	0.0177	1.32 KB
#3048	`SendReceive`	net472	2.34μs	3.02ns	11.3ns	0.218	1.37 KB
#3048	`SendReceive`	netcoreapp3.1	1.8μs	1.76ns	6.83ns	0.018	1.32 KB

Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	5.01μs	1.37ns	5.3ns	0.354	2.23 KB
master	`EnrichedLog`	netcoreapp3.1	4.26μs	2.74ns	10.3ns	0.0236	1.8 KB
#3048	`EnrichedLog`	net472	5.16μs	3.71ns	14.4ns	0.353	2.23 KB
#3048	`EnrichedLog`	netcoreapp3.1	4.26μs	1.28ns	4.79ns	0.0234	1.8 KB

Benchmarks.Trace.SpanBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`StartFinishSpan`	net472	1.12μs	0.423ns	1.58ns	0.128	810 B
master	`StartFinishSpan`	netcoreapp3.1	919ns	0.317ns	1.23ns	0.0101	760 B
master	`StartFinishScope`	net472	1.37μs	0.299ns	1.16ns	0.141	891 B
master	`StartFinishScope`	netcoreapp3.1	1.05μs	0.281ns	1.05ns	0.0121	880 B
#3048	`StartFinishSpan`	net472	1.11μs	0.559ns	2.01ns	0.128	810 B
#3048	`StartFinishSpan`	netcoreapp3.1	949ns	0.255ns	0.955ns	0.0101	760 B
#3048	`StartFinishScope`	net472	1.46μs	0.412ns	1.54ns	0.141	891 B
#3048	`StartFinishScope`	netcoreapp3.1	1.1μs	0.387ns	1.5ns	0.012	880 B

Benchmarks.Trace.TraceAnnotationsBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`RunOnMethodBegin`	net472	1.51μs	0.321ns	1.24ns	0.141	891 B
master	`RunOnMethodBegin`	netcoreapp3.1	1.19μs	0.529ns	2.05ns	0.0119	880 B
#3048	`RunOnMethodBegin`	net472	1.53μs	0.482ns	1.87ns	0.141	891 B
#3048	`RunOnMethodBegin`	netcoreapp3.1	1.2μs	0.586ns	2.19ns	0.012	880 B

andrewlock · 2022-08-26T22:00:48Z

Code Coverage Report 📊

✔️ Merging #3048 into master will not change line coverage
✔️ Merging #3048 into master will not change branch coverage
⛔ Merging #3048 into master will will increase complexity by 34

	master	#3048	Change
Lines	`17154` / `23534`	`17217` / `23578`
Lines %	`73%`	`73%`	`0%` ✔️
Branches	`10195` / `14538`	`10249` / `14568`
Branches %	`70%`	`70%`	`0%` ✔️
Complexity	`15640`	`15674`	`34` ⛔

View the full report for further details:

Datadog.Trace Breakdown ✔️

	master	#3048	Change
Lines %	`73%`	`73%`	`0%` ✔️
Branches %	`70%`	`70%`	`0%` ✔️
Complexity	`15640`	`15674`	`34` ⛔

The following classes have significant coverage changes.

File	Line coverage change	Branch coverage change	Complexity change
Datadog.Trace.Ci.GitInfo	`-17%` ⛔	`-11%` ⛔	`0` ✔️
Datadog.Trace.Agent.DiscoveryService.DiscoveryService	`-8%` ⛔	`0%` ✔️	`0` ✔️
Datadog.Trace.Activity.DiagnosticSourceEventListener	`-6%` ⛔	`0%` ✔️	`0` ✔️
Datadog.Trace.Ci.CIVisibility	`5%` ✔️	`6%` ✔️	`0` ✔️
Datadog.Trace.Agent.NullStatsAggregator	`8%` ✔️	`0%` ✔️	`1` ⛔
Datadog.Trace.Sampling.GlobalSamplingRule	`100%` ✔️	`0%` ✔️	`0` ✔️

View the full reports for further details:

In #3048, the logic for keeping a trace chunk occurred twice: once in the TraceContext and then again in the StatsAggregator. This PR moves all of the logic into the StatsAggregator. Main changes - Adds a new method to run all the trace chunk sampling logic: `bool IStatsAggregator.ShouldKeepTrace(ArraySegment trace)` - Adds a new interface to cleanly define each rule: `Datadog.Trace.Agent.ITraceSampler` - The one required method is: `bool Sample(ArraySegment trace)` - Derived types include: - `AnalyticsEventSampler` - `ErrorSampler` - `PrioritySampler` - `RareSampler` - Fixes the following bugs identified in the previous PR and adds regression tests: - Spans caught by the RareSampler must have the metric `_dd.rare=1` - Ensure that traces with errors (possibly in child spans) always get reported to the trace agent - Note: the trace agent has a more nuanced ErrorSampler, but this works for now and still eliminates a great deal of traffic

zacharycmontoya added area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) area:tests unit tests, integration tests labels Aug 3, 2022

zacharycmontoya requested review from a team as code owners August 3, 2022 22:29

zacharycmontoya self-assigned this Aug 3, 2022

zacharycmontoya added the type:enhancement Improvement to an existing feature label Aug 3, 2022

This was referenced Aug 3, 2022

[Tracer] Stats Computation: Disable stats computation and/or dropping P0 traces when the agent is not compatible #3049

Closed

[Tracer] Add endpoint discovery and dropping of P0 traces to stats computation feature #2988

Closed

This comment has been minimized.

Sign in to view

pierotibou approved these changes Aug 4, 2022

View reviewed changes

andrewlock approved these changes Aug 8, 2022

View reviewed changes

pierotibou reviewed Aug 16, 2022

View reviewed changes

Base automatically changed from zach/stats/existing-tests to master August 23, 2022 18:34

zacharycmontoya requested a review from a team as a code owner August 23, 2022 18:34

zacharycmontoya force-pushed the zach/stats/drops-p0s branch from 48aca98 to efe10a3 Compare August 25, 2022 15:53

andrewlock approved these changes Aug 25, 2022

View reviewed changes

zacharycmontoya added 8 commits August 25, 2022 11:33

Add ability to drop P0 spans.

67e058d

Caveat: We may have the StatsComputationSetting enabled but the agent we try to reach does not support it. That complicated mess will be addressed next.

Implement and test that we send the headers "Datadog-Client-Dropped-P…

6fd6876

…0-Traces" and "Datadog-Client-Dropped-P0-Spans" when we drop P0 traces

Delete one TODO that was sitting around in the code

6680e26

Adds a basic "rare" sampler that's used when dropping P0 traces. If a…

646096f

… stats point is completely new or the stats point contains an error, force the trace to be serialized to the trace agent.

Implement feedback on the TraceContext.ShouldKeepSpan implementation

86170e2

Address feedback in AgentWriter.cs

93c8702

Address feedback in Api.cs

f017ec6

zacharycmontoya added 2 commits August 25, 2022 11:48

Implement suggestion to update IStatsAggregator.AddRange(Span[] spans…

dfa3b59

…, int offset, int count) => IStatsAggregator.AddRange(ArraySegment spans)

Fixup test changes for the api header changes

be71919

zacharycmontoya force-pushed the zach/stats/drops-p0s branch from efe10a3 to be71919 Compare August 25, 2022 19:04

This comment has been minimized.

Sign in to view

Fix bad rebase

e583e02

This comment has been minimized.

Sign in to view

zacharycmontoya added 5 commits August 25, 2022 17:05

Test fix: Revert Datadog.Trace public api change

ccbbdc7

Fix unused statsComputationEnabled parameter in the Api constructor

7bad5cc

Test fix: Fix AgentWriter.CanComputeStats null reference exception

550341e

Test fix: Fix TestRunners.ValidNames for local testing

09e4aa4

Test fix: Fix TraceContextTests.SetAASContext

d9f6978

This comment has been minimized.

Sign in to view

Test fix: Make StatsTests more reliable

47a7dec

This comment has been minimized.

Sign in to view

Test fix: Hopefully the last fix to get StatsTests to be reliable, th…

81a60d7

…is time changing the way the duration is tested. Please work

This comment has been minimized.

Sign in to view

Test fix: Force stats to stop sending by flushing and closing

58a40f7

zacharycmontoya force-pushed the zach/stats/drops-p0s branch from d2bb1c6 to 58a40f7 Compare August 26, 2022 19:22

This comment has been minimized.

Sign in to view

Try to reduce flake, again, my increasing the stats interval back to …

4e71a0c

…the default value of 10. Now that we only wait once at the end, this should be acceptable

zacharycmontoya merged commit b480f0a into master Aug 26, 2022

zacharycmontoya deleted the zach/stats/drops-p0s branch August 26, 2022 23:15

github-actions Bot added this to the vNext milestone Aug 26, 2022

zacharycmontoya mentioned this pull request Aug 27, 2022

[Tracer] Refactor stats computation into StatsAggregator #3133

Merged


		ArraySegment<Span> spansToWrite = default;

		bool shouldKeepSpan = true;


		private static readonly IDatadogLogger Log = DatadogLogging.GetLoggerFor<StatsAggregator>();

		private readonly HashSet<StatsAggregationKey> _keys;

		var state = new SendTracesState(traces, numberOfTraces, statsComputationEnabled, numberOfDroppedP0Traces, numberOfDroppedP0Spans);

		return SendWithRetry(_tracesEndpoint, _sendTraces, state);

Uh oh!

Conversation

zacharycmontoya commented Aug 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Reason for change

Implementation details

TraceContext changes

AgentWriter / IApi changes

Test coverage

Other details

Remaining Work Items

Uh oh!

This comment has been minimized.

This comment has been minimized.

pierotibou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zacharycmontoya Aug 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zacharycmontoya commented Aug 4, 2022

Uh oh!

bouwkast commented Aug 4, 2022

Uh oh!

zacharycmontoya commented Aug 5, 2022

Uh oh!

zacharycmontoya commented Aug 5, 2022

Uh oh!

pierotibou commented Aug 5, 2022

Uh oh!

andrewlock left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewlock left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

zacharycmontoya commented Aug 3, 2022 •

edited

Loading

zacharycmontoya Aug 4, 2022 •

edited

Loading