[Tracer] Ignore more assemblies and don't resend them all at each run by pierotibou · Pull Request #2966 · DataDog/dd-trace-dotnet

pierotibou · 2022-07-11T08:33:10Z

Summary of changes

Ignore assembly names like ℛ*fe48e1c0-b9fb-433d-a781-5f0f0b6555e4#2-0 as there were 6000 of them. Also do not resend all assemblies at each run.

Reason for change

There was a misunderstanding with R&P, we need to send them once, not resend the whole package every time. We'll need to do the same for config and integrations.

Implementation details

For the update, rebuild a ConcurrentDictionnary at each upload.

Test coverage

Added 2 test cases

Other details

andrewlock

I initially thought this implementation would be sufficient, but actually I don't think it will be. With this design we will still send duplicate assemblies if there are multiple domains. We still have the global uniqueness requirement, so maybe something like this would work:

private readonly ConcurrentDictionary<DependencyTelemetryData, bool> _allAssemblies = new();
private ConcurrentBag<DependencyTelemetryData> _currentAssemblies = new();

internal void AssemblyLoaded(AssemblyName assembly)
{
    // ...
    if(_allAssemblies.TryAdd(key, true))
    {
        _currentAssemblies.TryAdd(key))
    }
}

public ICollection<DependencyTelemetryData> GetData()
{
    var assemblies = Interlocked.Exchange(ref _currentAssemblies , new ConcurrentBag<DependencyTelemetryData>());
    // ...
}

Also this will break all the integration test telemetry checks I expect, so they will need updating too.

kevingosse · 2022-07-11T09:33:27Z

I initially thought this implementation would be sufficient, but actually I don't think it will be. With this design we will still send duplicate assemblies if there are multiple domains. We still have the global uniqueness requirement, so maybe something like this would work:
private readonly ConcurrentDictionary<DependencyTelemetryData, bool> _allAssemblies = new();
private ConcurrentBag<DependencyTelemetryData> _currentAssemblies = new();

internal void AssemblyLoaded(AssemblyName assembly)
{
    // ...
    if(_allAssemblies.TryAdd(key, true))
    {
        _currentAssemblies.TryAdd(key))
    }
}

public ICollection<DependencyTelemetryData> GetData()
{
    var assemblies = Interlocked.Exchange(ref _currentAssemblies , new ConcurrentBag<DependencyTelemetryData>());
    // ...
}
Also this will break all the integration test telemetry checks I expect, so they will need updating too.

Never ever use ConcurrentBag without a compelling use-case and setting up some benchmarks. Please use ConcurrentQueue instead 🙂

pierotibou · 2022-08-11T13:28:56Z

Was a test.

lucaspimentel · 2022-09-08T18:33:22Z

            }

-            return _assemblies.Keys;
+            var assembliesToRecord = new List<DependencyTelemetryData>();


[heap allocations] Is this GetData() method called often? How hard would it be to track the count of new assemblies so you could initialize the list with the correct count? Or even use an array instead of a List<T>?

var assembliesToRecord = new List<DependencyTelemetryData>(newAssemblyCount); // ...or... var assembliesToRecord = new DependencyTelemetryData[newAssemblyCount];

Is this GetData() method called often?

Every 1 minute (max)

How hard would it be to track the count of new assemblies

Moderately hard I think with the current design due to concurrency. The trouble is that assemblies could be added to the underlying dictionary while the following loop is created.

Alternative suggestion - I believe (please double check) calls to this method are single-threaded. So you could do something like this:

private List<DependencyTelemetryData>? _toSend; public ICollection<DependencyTelemetryData> GetData() { // ... _toSend ??= new List<DependencyTelemetryData>(_assemblies.Count); _toSend.Clear(); foreach (var assembly in _assemblies) //...

That way you initialize to the initial set, grow as necessary, but don't create a new list each time

andrewlock

Spotted an issue in how we handle errors :(

andrewlock · 2022-09-09T13:04:21Z

+            foreach (var assembly in _assemblies)
+            {
+                if (assembly.Value == false)
+                {
+                    _assemblies[assembly.Key] = true;
+                    assembliesToRecord.Add(assembly.Key);
+                }
+            }


Just spotted an issue with this. The code in

dd-trace-dotnet/tracer/src/Datadog.Trace/Telemetry/TelemetryController.cs

Line 216 in 4d6154c

var dependencies = _dependencies.GetData() ?? _circuitBreaker.PreviousDependencies;

assumes that GetData() returns all the assemblies, so it can tolerate transport errors.

With this new approach, you could have the following:

Initial GetData() returns 50 assemblies

Sending fails. TelemetryController remembers failed list

A new assembly is referenced

Next clock, TelemetryController calls GetData().

This returns the single assembly. TelemetryController throws away the stale data.

We never record the original 50 assemblies

So something has to change 🙁

One possible option - keep this code the same, but change this line to something like this:

var dependencies = _dependencies.GetData(); if(dependencies is null) { dependencies = _circuitBreaker.PreviousDependencies; } else if(_circuitBreaker.PreviousDependencies is {} oldDeps) { dependencies.AddRange(oldDeps); }

Thanks a lot for spotting this and for the suggestion.
Done in 66e8c76

andrewlock · 2022-09-22T11:14:35Z

Benchmarks Report 🐌

Benchmarks for #2966 compared to master:

All benchmarks have the same speed
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	717μs	600ns	2.32μs	0.361	3.18 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	476μs	150ns	539ns	0	2.58 KB
#2966	`WriteAndFlushEnrichedTraces`	net472	713μs	381ns	1.42μs	0.357	3.18 KB
#2966	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	478μs	115ns	432ns	0	2.58 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`AllCycleSimpleBody`	net472	213ns	0.118ns	0.456ns	0.0675	0	425 B
master	`AllCycleSimpleBody`	netcoreapp3.1	251ns	0.385ns	1.49ns	0.00585	0	424 B
master	`AllCycleMoreComplexBody`	net472	212ns	0.0923ns	0.346ns	0.0637	0	401 B
master	`AllCycleMoreComplexBody`	netcoreapp3.1	250ns	0.22ns	0.851ns	0.00541	0	400 B
master	`BodyExtractorSimpleBody`	net472	278ns	0.0937ns	0.338ns	0.0573	0	361 B
master	`BodyExtractorSimpleBody`	netcoreapp3.1	233ns	0.247ns	0.923ns	0.00372	0	272 B
master	`BodyExtractorMoreComplexBody`	net472	15.8μs	14.6ns	56.7ns	1.21	0.0158	7.62 KB
master	`BodyExtractorMoreComplexBody`	netcoreapp3.1	12.4μs	4.22ns	15.2ns	0.0931	0	6.75 KB
#2966	`AllCycleSimpleBody`	net472	199ns	0.0373ns	0.129ns	0.0675	0	425 B
#2966	`AllCycleSimpleBody`	netcoreapp3.1	256ns	0.136ns	0.525ns	0.00585	0	424 B
#2966	`AllCycleMoreComplexBody`	net472	202ns	0.234ns	0.875ns	0.0638	0	401 B
#2966	`AllCycleMoreComplexBody`	netcoreapp3.1	258ns	0.296ns	1.14ns	0.00544	0	400 B
#2966	`BodyExtractorSimpleBody`	net472	279ns	0.221ns	0.766ns	0.0574	0	361 B
#2966	`BodyExtractorSimpleBody`	netcoreapp3.1	225ns	0.108ns	0.404ns	0.00362	0	272 B
#2966	`BodyExtractorMoreComplexBody`	net472	15.9μs	14.4ns	55.7ns	1.21	0.0158	7.62 KB
#2966	`BodyExtractorMoreComplexBody`	netcoreapp3.1	12.1μs	5.68ns	22ns	0.0902	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	181μs	296ns	1.15μs	0.27	20.6 KB
#2966	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
#2966	`SendRequest`	netcoreapp3.1	179μs	224ns	869ns	0.268	20.58 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.85μs	5.97ns	22.3ns	0.15	0	947 B
master	`ExecuteNonQuery`	netcoreapp3.1	1.46μs	0.564ns	2.19ns	0.0124	0	936 B
#2966	`ExecuteNonQuery`	net472	1.77μs	0.621ns	2.41ns	0.15	0.000884	947 B
#2966	`ExecuteNonQuery`	netcoreapp3.1	1.43μs	3.45ns	12.9ns	0.0127	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.69μs	1.15ns	4.15ns	0.182	1.16 KB
master	`CallElasticsearch`	netcoreapp3.1	1.52μs	1.04ns	3.88ns	0.0145	1.1 KB
master	`CallElasticsearchAsync`	net472	2.8μs	1.53ns	5.71ns	0.204	1.29 KB
master	`CallElasticsearchAsync`	netcoreapp3.1	1.66μs	1.73ns	6.47ns	0.0165	1.22 KB
#2966	`CallElasticsearch`	net472	2.64μs	1.59ns	5.74ns	0.184	1.16 KB
#2966	`CallElasticsearch`	netcoreapp3.1	1.53μs	0.646ns	2.42ns	0.0146	1.1 KB
#2966	`CallElasticsearchAsync`	net472	2.69μs	2.19ns	8.48ns	0.205	1.29 KB
#2966	`CallElasticsearchAsync`	netcoreapp3.1	1.69μs	0.475ns	1.71ns	0.0169	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.86μs	2ns	7.48ns	0.225	1.42 KB
master	`ExecuteAsync`	netcoreapp3.1	1.82μs	1.09ns	4.08ns	0.0184	1.34 KB
#2966	`ExecuteAsync`	net472	2.84μs	2.56ns	9.57ns	0.224	1.42 KB
#2966	`ExecuteAsync`	netcoreapp3.1	1.8μs	1.02ns	3.82ns	0.018	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.88μs	3.85ns	14.9ns	0.438	2.77 KB
master	`SendAsync`	netcoreapp3.1	3.83μs	1.96ns	7.05ns	0.0348	2.6 KB
#2966	`SendAsync`	net472	5.86μs	2.84ns	11ns	0.44	2.77 KB
#2966	`SendAsync`	netcoreapp3.1	3.57μs	1.33ns	4.78ns	0.0356	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.16μs	4.33ns	16.8ns	0.288	1.81 KB
master	`EnrichedLog`	netcoreapp3.1	2.57μs	1.37ns	5.11ns	0.0255	1.85 KB
#2966	`EnrichedLog`	net472	3.12μs	2.05ns	7.93ns	0.287	1.81 KB
#2966	`EnrichedLog`	netcoreapp3.1	2.55μs	1.56ns	6.04ns	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	133ns	515ns	0.687	0.229	4.66 KB
master	`EnrichedLog`	netcoreapp3.1	117μs	262ns	979ns	0.0587	0	4.5 KB
#2966	`EnrichedLog`	net472	150μs	129ns	500ns	0.678	0.226	4.66 KB
#2966	`EnrichedLog`	netcoreapp3.1	118μs	211ns	816ns	0.0586	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	6.03μs	2.22ns	8.59ns	0.57	0.00302	3.59 KB
master	`EnrichedLog`	netcoreapp3.1	4.42μs	2.2ns	8.51ns	0.0527	0	3.91 KB
#2966	`EnrichedLog`	net472	5.91μs	2.89ns	11.2ns	0.568	0.00296	3.59 KB
#2966	`EnrichedLog`	netcoreapp3.1	4.44μs	2.42ns	9.35ns	0.0534	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.25μs	1.91ns	7.16ns	0.217	1.37 KB
master	`SendReceive`	netcoreapp3.1	1.78μs	0.714ns	2.57ns	0.018	1.32 KB
#2966	`SendReceive`	net472	2.28μs	2.57ns	9.95ns	0.217	1.37 KB
#2966	`SendReceive`	netcoreapp3.1	1.85μs	0.727ns	2.81ns	0.0176	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.1μs	1.32ns	4.77ns	0.353	2.23 KB
master	`EnrichedLog`	netcoreapp3.1	4.17μs	2.14ns	8.03ns	0.023	1.8 KB
#2966	`EnrichedLog`	net472	4.91μs	1.89ns	7.33ns	0.354	2.23 KB
#2966	`EnrichedLog`	netcoreapp3.1	4.29μs	3.29ns	12.7ns	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.18μs	0.506ns	1.82ns	0.129	810 B
master	`StartFinishSpan`	netcoreapp3.1	907ns	0.497ns	1.93ns	0.0101	760 B
master	`StartFinishScope`	net472	1.46μs	1.32ns	5.13ns	0.141	891 B
master	`StartFinishScope`	netcoreapp3.1	1.04μs	0.489ns	1.89ns	0.0119	880 B
#2966	`StartFinishSpan`	net472	1.18μs	0.901ns	3.49ns	0.129	810 B
#2966	`StartFinishSpan`	netcoreapp3.1	969ns	0.359ns	1.39ns	0.0102	760 B
#2966	`StartFinishScope`	net472	1.44μs	0.577ns	2.23ns	0.141	891 B
#2966	`StartFinishScope`	netcoreapp3.1	1.1μs	0.448ns	1.73ns	0.0122	880 B

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`RunOnMethodBegin`	net472	1.53μs	0.659ns	2.47ns	0.141	891 B
master	`RunOnMethodBegin`	netcoreapp3.1	1.18μs	0.755ns	2.82ns	0.0118	880 B
#2966	`RunOnMethodBegin`	net472	1.52μs	1.47ns	5.71ns	0.141	891 B
#2966	`RunOnMethodBegin`	netcoreapp3.1	1.16μs	0.318ns	1.23ns	0.0117	880 B

andrewlock · 2022-09-22T13:14:54Z

Code Coverage Report 📊

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

	master	#2966	Change
Lines	`18002` / `24930`	`18032` / `24950`
Lines %	`72%`	`72%`	`0%` ✔️
Branches	`10484` / `15274`	`10516` / `15302`
Branches %	`69%`	`69%`	`0%` ✔️
Complexity	`16497`	`16527`	`30` ⛔

View the full report for further details:

Datadog.Trace Breakdown ✔️

	master	#2966	Change
Lines %	`72%`	`72%`	`0%` ✔️
Branches %	`69%`	`69%`	`0%` ✔️
Complexity	`16497`	`16527`	`30` ⛔

The following classes have significant coverage changes.

File	Line coverage change	Branch coverage change	Complexity change
Datadog.Trace.Agent.Transports.HttpStreamRequest	`-3%` ⚠️	`-8%` ⛔	`3` ⛔

View the full reports for further details:

andrewlock

LGTM, though could apply the optimization to reuse the list

pierotibou requested a review from a team as a code owner July 11, 2022 08:33

andrewlock reviewed Jul 11, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

pierotibou force-pushed the pierre/tlm-ignore branch from 6a0afb1 to d52f09a Compare August 1, 2022 11:03

This comment has been minimized.

Sign in to view

andrewlock reviewed Aug 1, 2022

View reviewed changes

Comment thread tracer/src/Datadog.Trace/Telemetry/Collectors/DependencyTelemetryCollector.cs Outdated

andrewlock reviewed Aug 1, 2022

View reviewed changes

Comment thread tracer/src/Datadog.Trace/Telemetry/Collectors/DependencyTelemetryCollector.cs Outdated

This comment has been minimized.

Sign in to view

andrewlock approved these changes Sep 5, 2022

View reviewed changes

lucaspimentel added the area:telemetry label Sep 8, 2022

lucaspimentel reviewed Sep 8, 2022

View reviewed changes

lucaspimentel approved these changes Sep 8, 2022

View reviewed changes

andrewlock requested changes Sep 9, 2022

View reviewed changes

pierotibou added 5 commits September 22, 2022 11:50

Exclude other guid like assemblies

6b2c640

Don't send all dependencies all the time

e14ec6a

Disallow sending assemblies that have already been recorded

7181565

feedbacks

c5ab57a

aggregate with previous dependencies if need be

66e8c76

pierotibou force-pushed the pierre/tlm-ignore branch from 2aa638d to 66e8c76 Compare September 22, 2022 10:45

andrewlock approved these changes Sep 23, 2022

View reviewed changes

pierotibou merged commit f3430f3 into master Sep 23, 2022

pierotibou deleted the pierre/tlm-ignore branch September 23, 2022 09:13

github-actions Bot added this to the vNext milestone Sep 23, 2022

Uh oh!

Conversation

pierotibou commented Jul 11, 2022

Summary of changes

Reason for change

Implementation details

Test coverage

Other details

Uh oh!

andrewlock left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

kevingosse commented Jul 11, 2022

Uh oh!

This comment has been minimized.

This comment has been minimized.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

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

Uh oh!

Uh oh!

lucaspimentel Sep 8, 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!

andrewlock Sep 9, 2022

Choose a reason for hiding this comment

Uh oh!

andrewlock left a comment

Choose a reason for hiding this comment

Uh oh!

andrewlock Sep 9, 2022

Choose a reason for hiding this comment

Uh oh!

pierotibou Sep 22, 2022

Choose a reason for hiding this comment

Uh oh!

andrewlock commented Sep 22, 2022

Benchmarks Report 🐌

Benchmark details

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Uh oh!

andrewlock commented Sep 22, 2022

Code Coverage Report 📊

Datadog.Trace Breakdown ✔️

Uh oh!

andrewlock left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pierotibou commented Aug 11, 2022 •

edited

Loading

lucaspimentel Sep 8, 2022 •

edited

Loading