Data Streams Monitoring support in Kinesis#6428
Conversation
e4ef173 to
6d146a2
Compare
Datadog ReportBranch report: ✅ 0 Failed, 464472 Passed, 3674 Skipped, 32h 59m 38.8s Total Time |
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing the following branches/commits: Execution-time benchmarks measure the whole time it takes to execute a program. And are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are shown in red. The following thresholds were used for comparing the execution times:
Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard. Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph). gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6428) - mean (68ms) : 66, 71
. : milestone, 68,
master - mean (68ms) : 65, 71
. : milestone, 68,
section CallTarget+Inlining+NGEN
This PR (6428) - mean (978ms) : 952, 1003
. : milestone, 978,
master - mean (980ms) : 952, 1008
. : milestone, 980,
gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6428) - mean (107ms) : 105, 110
. : milestone, 107,
master - mean (107ms) : 105, 109
. : milestone, 107,
section CallTarget+Inlining+NGEN
This PR (6428) - mean (679ms) : 664, 694
. : milestone, 679,
master - mean (679ms) : 663, 695
. : milestone, 679,
gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6428) - mean (91ms) : 89, 93
. : milestone, 91,
master - mean (91ms) : 89, 93
. : milestone, 91,
section CallTarget+Inlining+NGEN
This PR (6428) - mean (631ms) : 614, 648
. : milestone, 631,
master - mean (633ms) : 616, 650
. : milestone, 633,
gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6428) - mean (193ms) : 189, 198
. : milestone, 193,
master - mean (193ms) : 188, 198
. : milestone, 193,
section CallTarget+Inlining+NGEN
This PR (6428) - mean (1,100ms) : 1073, 1127
. : milestone, 1100,
master - mean (1,098ms) : 1072, 1123
. : milestone, 1098,
gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6428) - mean (279ms) : 274, 283
. : milestone, 279,
master - mean (277ms) : 273, 281
. : milestone, 277,
section CallTarget+Inlining+NGEN
This PR (6428) - mean (869ms) : 846, 891
. : milestone, 869,
master - mean (868ms) : 840, 895
. : milestone, 868,
gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6428) - mean (266ms) : 261, 271
. : milestone, 266,
master - mean (266ms) : 262, 271
. : milestone, 266,
section CallTarget+Inlining+NGEN
This PR (6428) - mean (847ms) : 808, 886
. : milestone, 847,
master - mean (853ms) : 824, 881
. : milestone, 853,
|
Throughput/Crank Report ⚡Throughput results for AspNetCoreSimpleController comparing the following branches/commits: Cases where throughput results for the PR are worse than latest master (5% drop or greater), results are shown in red. Note that these results are based on a single point-in-time result for each branch. For full results, see one of the many, many dashboards! gantt
title Throughput Linux x64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (6428) (11.069M) : 0, 11068775
master (11.244M) : 0, 11243896
benchmarks/2.9.0 (11.033M) : 0, 11032866
section Automatic
This PR (6428) (7.340M) : 0, 7339967
master (7.201M) : 0, 7201119
benchmarks/2.9.0 (7.786M) : 0, 7785853
section Trace stats
master (7.576M) : 0, 7576229
section Manual
master (11.196M) : 0, 11196081
section Manual + Automatic
This PR (6428) (6.726M) : 0, 6726459
master (6.638M) : 0, 6638437
section DD_TRACE_ENABLED=0
master (10.422M) : 0, 10421912
gantt
title Throughput Linux arm64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (6428) (9.259M) : 0, 9258827
master (9.372M) : 0, 9372216
benchmarks/2.9.0 (9.495M) : 0, 9494821
section Automatic
This PR (6428) (6.320M) : 0, 6320356
master (6.384M) : 0, 6383601
section Trace stats
master (6.655M) : 0, 6655008
section Manual
master (9.413M) : 0, 9413276
section Manual + Automatic
This PR (6428) (5.968M) : 0, 5967935
master (5.896M) : 0, 5895709
section DD_TRACE_ENABLED=0
master (8.890M) : 0, 8890260
gantt
title Throughput Windows x64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (6428) (9.721M) : 0, 9720629
master (9.902M) : 0, 9901561
benchmarks/2.9.0 (10.020M) : 0, 10019592
section Automatic
This PR (6428) (6.261M) : crit ,0, 6260570
master (6.645M) : 0, 6645144
benchmarks/2.9.0 (7.255M) : 0, 7255257
section Trace stats
master (7.167M) : 0, 7167171
section Manual
master (10.054M) : 0, 10054191
section Manual + Automatic
This PR (6428) (5.850M) : 0, 5850441
master (6.071M) : 0, 6070705
section DD_TRACE_ENABLED=0
master (9.440M) : 0, 9439995
|
Benchmarks Report for tracer 🐌Benchmarks for #6428 compared to master:
The following thresholds were used for comparing the benchmark speeds:
Allocation changes below 0.5% are ignored. Benchmark detailsBenchmarks.Trace.ActivityBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.ElasticsearchBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.SpanBenchmark - Slower
|
| Benchmark | diff/base | Base Median (ns) | Diff Median (ns) | Modality |
|---|---|---|---|---|
| Benchmarks.Trace.SpanBenchmark.StartFinishScope‑net6.0 | 1.130 | 480.46 | 542.70 |
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | StartFinishSpan |
net6.0 | 417ns | 0.297ns | 1.15ns | 0.00801 | 0 | 0 | 576 B |
| master | StartFinishSpan |
netcoreapp3.1 | 610ns | 0.354ns | 1.37ns | 0.00765 | 0 | 0 | 576 B |
| master | StartFinishSpan |
net472 | 635ns | 0.369ns | 1.43ns | 0.0916 | 0 | 0 | 578 B |
| master | StartFinishScope |
net6.0 | 481ns | 0.364ns | 1.41ns | 0.00962 | 0 | 0 | 696 B |
| master | StartFinishScope |
netcoreapp3.1 | 735ns | 0.449ns | 1.74ns | 0.00924 | 0 | 0 | 696 B |
| master | StartFinishScope |
net472 | 880ns | 0.576ns | 2.23ns | 0.104 | 0 | 0 | 658 B |
| #6428 | StartFinishSpan |
net6.0 | 421ns | 0.195ns | 0.753ns | 0.00806 | 0 | 0 | 576 B |
| #6428 | StartFinishSpan |
netcoreapp3.1 | 558ns | 0.51ns | 1.98ns | 0.0079 | 0 | 0 | 576 B |
| #6428 | StartFinishSpan |
net472 | 643ns | 0.377ns | 1.46ns | 0.0918 | 0 | 0 | 578 B |
| #6428 | StartFinishScope |
net6.0 | 543ns | 0.172ns | 0.645ns | 0.00979 | 0 | 0 | 696 B |
| #6428 | StartFinishScope |
netcoreapp3.1 | 747ns | 0.636ns | 2.46ns | 0.00975 | 0 | 0 | 696 B |
| #6428 | StartFinishScope |
net472 | 850ns | 0.551ns | 2.14ns | 0.104 | 0 | 0 | 658 B |
Benchmarks.Trace.TraceAnnotationsBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | RunOnMethodBegin |
net6.0 | 619ns | 0.27ns | 1.04ns | 0.00987 | 0 | 0 | 696 B |
| master | RunOnMethodBegin |
netcoreapp3.1 | 942ns | 0.798ns | 3.09ns | 0.00934 | 0 | 0 | 696 B |
| master | RunOnMethodBegin |
net472 | 1.11μs | 0.544ns | 2.04ns | 0.104 | 0 | 0 | 658 B |
| #6428 | RunOnMethodBegin |
net6.0 | 615ns | 0.371ns | 1.39ns | 0.00984 | 0 | 0 | 696 B |
| #6428 | RunOnMethodBegin |
netcoreapp3.1 | 930ns | 0.628ns | 2.35ns | 0.0092 | 0 | 0 | 696 B |
| #6428 | RunOnMethodBegin |
net472 | 1.07μs | 0.569ns | 2.2ns | 0.104 | 0 | 0 | 658 B |
3e8e01e to
afff658
Compare
* Added DSM checkpoints to PutRecord/PutRecords * Added instrumentation to GetRecords
afff658 to
391fcce
Compare
|
The test failures look unrelated and are failing for most other recent changes in the repo as well -- windows integ test occasionally and system_tests and friends from going past the 60-minute limit. The unit tests and non-windows integration tests all succeeded and there is nothing platform-specific in these changes, so I do not expect these failing tests have missed coverage for anything significant. |
|
The CI run after the minor changes following up on Igor's comments passed all integ tests and had one failure in Linux unit tests, neither of which should have been impacted by the small changes in that commit, which makes me more confident that the test failures here are not a result of this change. |
I haven't reviewed the whole change yet, but I don't get why we don't do the ARN parsing for the producer too 🤔 |
| var dataStreamsManager = Tracer.Instance.TracerManager.DataStreamsManager; | ||
| if (dataStreamsManager != null && dataStreamsManager.IsEnabled) | ||
| { | ||
| var edgeTags = new[] { "direction:out", $"topic:{streamName}", "type:kinesis" }; |
There was a problem hiding this comment.
If streamName is nullable, shouldn't we add it only if it has a value ?
There was a problem hiding this comment.
This is inside an outer block of if (scope?.Span.Context != null && !string.IsNullOrEmpty(streamName)) so it shouldn't get here if streamName has no value.
The existing producer instrumentation supports only name, unlike other languages (like java) which support only ARN. The dotnet API allows either on producer, although ARN is preferred. This PR doesn't change that producer behavior (which would be the potentially-breaking change I refer to) and uses stream name as the DSM node name. The consumer API allows only ARN, so the newly-added consumer has to support ARN. In order to be compatible with the dotnet producer instrumentation, the new consumer instrumentation extracts stream name from the ARN and uses that as the DSM node name. |
|
|
||
| return retval; | ||
| // Convert to Dictionary<string, object> to satisfy IHeadersCollection | ||
| return headers.ToDictionary(kvp => kvp.Key, kvp => (object)kvp.Value); |
There was a problem hiding this comment.
btw I was wondering how often is this called ? Would it make sense to directly have a dict of <string, object> and make the casts when we need to modify the list ? This would allow us to avoid making a copy of the dictionary here...
There was a problem hiding this comment.
The main path for this will be to create the adapter, add a small (<10) number of single entries, use it once, and destroy it.
The DSM manager is built around the expectation of some persistent headers that it can manipulate through the adapter interface, but because kinesis doesn't have headers or metadata this class is just wrapping a one-off that doesn't persist beyond one scope.
There was a problem hiding this comment.
Yeah, this could be a (small) performance issue if it's used in every Kinesis call. Not only is it allocating a new dictionary every time, but it's doing it using a LINQ call (IEnumerable<KeyValuePair<string, List<string>>>.ToDictionary) which will itself probably allocate an enumerator as well and cause the dictionary to grow as needed, which is wasteful.
If nothing else, I would at least remove the LINQ code and pre-initialize the new dictionary to the required size. Something like:
var result = new Dictionary<string, object>(capacity: headers.Count);
foreach (var kvp in headers)
{
result[kvp.Key] = kvp.Value;
}
return result;It would be even better if the result was cacheable, or if it returned a sort of wrapper around headers instead of an entirely new object.
bouwkast
left a comment
bouwkast
left a comment
There was a problem hiding this comment.
Could we add integrations tests for enabling Data Streams Monitoring for Kinesis?
Right now the testing of this seems to be a bit light.
For example:
vandonr
left a comment
vandonr
left a comment
There was a problem hiding this comment.
LGTM minus the tests, I gotta agree with Steven that we should have a test for Kinesis with DSM enabled.
It could be just an extra parameter to AwsKinesisTests maybe ?
| return null; | ||
| } | ||
|
|
||
| var arnComponents = arn!.Split('/'); |
There was a problem hiding this comment.
[performance] Not necessarily a blocker (we could come back and clean it up in a separate PR later), but if this code is in the app's hot path, we should do this in a way that doesn't allocate the arnComponents string array.
One approach could be to use IndexOf('/') twice. We also have a string.SplitIntoSpans() helper that splits a string into Span<char> tokens and enumerate over them.
Here's an example that uses both methods:
dd-trace-dotnet/tracer/src/Datadog.Trace/Propagators/W3CBaggagePropagator.cs
Lines 250 to 256 in eed7b60
There was a problem hiding this comment.
I've added this to DSMON-490 tracking followup improvements, planned to be a fast-follow after the freeze.
bouwkast
left a comment
bouwkast
left a comment
There was a problem hiding this comment.
Thanks!
As long as the tests pass LGTM
|
|
||
| var scope = AwsKinesisCommon.CreateScope(Tracer.Instance, Operation, SpanKinds.Consumer, null, out var tags); | ||
|
|
||
| string? streamName = AwsKinesisCommon.StreamNameFromARN(request.StreamARN); |
There was a problem hiding this comment.
Not a blocker for this PR, but I think we can skip this call if the integration is disabled. (if scope or tags is null)
|
|
||
| var scope = AwsKinesisCommon.CreateScope(Tracer.Instance, Operation, SpanKinds.Producer, null, out var tags); | ||
|
|
||
| string? streamName = AwsKinesisCommon.StreamNameFromARN(request.StreamARN); |
There was a problem hiding this comment.
Same comment as above about skipping AwsKinesisCommon.StreamNameFromARN() is not needed.
| return Convert.FromBase64String(value[value.Count - 1]); | ||
| } | ||
|
|
||
| return new byte[0]; |
There was a problem hiding this comment.
We aggressively try to avoid heap allocations. This returns the same array on every call instead of creating a new one:
| return new byte[0]; | |
| return Array.Empty<byte>(); |
But C# these days has a newer syntax that will do the right thing:
| return new byte[0]; | |
| return []; |
lucaspimentel
left a comment
lucaspimentel
left a comment
There was a problem hiding this comment.
LGTM aside from a few non-blocking comments regarding heap allocations.
|
andrewlock
added
type:new-feature
area:automatic-instrumentation
## Summary of changes * Added Data Streams Monitoring checkpoints to existing autoinstrumentation of Kinesis PutRecord[s][Async] * Added autoinstrumentation to Kinesis GetRecords[Async] ## Reason for change Data Streams Monitoring was not supported for Kinesis, this adds that functionality. ## Implementation details The existing producer instrumentation only works when StreamName is specified, not when StreamARN is specified. These changes leave that behavior unchanged at present. The new consumer instrumentation is able to infer a StreamName from the [mandatory] StreamARN so that the producer and consumer can use the same naming system. The reverse would not be possible, and changing producer instrumentation to use ARNs instead would be a potentially-breaking change to existing users of the functionality so is outside the scope of this feature addition.
6 participants
Summary of changes
Reason for change
Data Streams Monitoring was not supported for Kinesis, this adds that functionality.
Implementation details
The existing producer instrumentation only works when StreamName is specified, not when StreamARN is specified. These changes leave that behavior unchanged at present.
The new consumer instrumentation is able to infer a StreamName from the [mandatory] StreamARN so that the producer and consumer can use the same naming system. The reverse would not be possible, and changing producer instrumentation to use ARNs instead would be a potentially-breaking change to existing users of the functionality so is outside the scope of this feature addition.
Test coverage
Other details