Closed bouwkast closed 19 hours ago
The following differences have been observed in committed snapshots. It is meant to help the reviewer. The diff is simplistic, so please check some files anyway while we improve it.
1 occurrences of :
+ },
+ {
+ TraceId: Id_60,
+ SpanId: Id_61,
+ Name: internal,
+ Resource: TimeParent,
+ Service: Samples.NetActivitySdk,
+ Type: custom,
+ Tags: {
+ env: integration_tests,
+ language: dotnet,
+ otel.status_code: STATUS_CODE_UNSET,
+ runtime-id: Guid_2,
+ span.kind: internal,
+ version: 1.0.0
+ },
+ Metrics: {
+ process_id: 0,
+ _dd.top_level: 1.0,
+ _dd.tracer_kr: 1.0,
+ _sampling_priority_v1: 1.0
+ }
+ },
+ {
+ TraceId: Id_60,
+ SpanId: Id_62,
+ Name: internal,
+ Resource: TimeTrigger,
+ Service: Samples.NetActivitySdk,
+ Type: custom,
+ ParentId: Id_61,
+ Tags: {
+ env: integration_tests,
+ language: dotnet,
+ otel.status_code: STATUS_CODE_UNSET,
+ span.kind: internal,
+ version: 1.0.0
+ }
+ },
+ {
+ TraceId: Id_60,
+ SpanId: Id_63,
+ Name: internal,
+ Resource: TimeChild,
+ Service: Samples.NetActivitySdk,
+ Type: custom,
+ ParentId: Id_62,
+ Tags: {
+ env: integration_tests,
+ language: dotnet,
+ otel.status_code: STATUS_CODE_UNSET,
+ span.kind: internal,
+ version: 1.0.0
+ }
Branch report: steven/activity-null-id
Commit report: 17eab6a
Test service: dd-trace-dotnet
:white_check_mark: 0 Failed, 344820 Passed, 1649 Skipped, 14h 28m 18.03s Total Time :snowflake: 1 New Flaky
HttpClient_SubmitsTraces
- Datadog.Trace.ClrProfiler.IntegrationTests.HttpMessageHandlerTests
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 (5739) - mean (72ms) : 65, 80
. : milestone, 72,
master - mean (74ms) : 65, 83
. : milestone, 74,
section CallTarget+Inlining+NGEN
This PR (5739) - mean (990ms) : 970, 1010
. : milestone, 990,
master - mean (991ms) : 973, 1009
. : milestone, 991,
gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5739) - mean (109ms) : 106, 112
. : milestone, 109,
master - mean (110ms) : 107, 113
. : milestone, 110,
section CallTarget+Inlining+NGEN
This PR (5739) - mean (703ms) : 680, 725
. : milestone, 703,
master - mean (698ms) : 675, 722
. : milestone, 698,
gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5739) - mean (92ms) : 89, 96
. : milestone, 92,
master - mean (93ms) : 90, 96
. : milestone, 93,
section CallTarget+Inlining+NGEN
This PR (5739) - mean (654ms) : 629, 679
. : milestone, 654,
master - mean (657ms) : 630, 683
. : milestone, 657,
gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5739) - mean (193ms) : 189, 198
. : milestone, 193,
master - mean (192ms) : 188, 196
. : milestone, 192,
section CallTarget+Inlining+NGEN
This PR (5739) - mean (1,098ms) : 1075, 1121
. : milestone, 1098,
master - mean (1,081ms) : 1059, 1103
. : milestone, 1081,
gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5739) - mean (278ms) : 272, 284
. : milestone, 278,
master - mean (276ms) : 271, 281
. : milestone, 276,
section CallTarget+Inlining+NGEN
This PR (5739) - mean (886ms) : 863, 910
. : milestone, 886,
master - mean (874ms) : 849, 899
. : milestone, 874,
gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5739) - mean (267ms) : 261, 272
. : milestone, 267,
master - mean (269ms) : 263, 275
. : milestone, 269,
section CallTarget+Inlining+NGEN
This PR (5739) - mean (862ms) : 836, 889
. : milestone, 862,
master - mean (867ms) : 839, 894
. : milestone, 867,
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 (5739) (12.154M) : 0, 12153787
master (11.753M) : 0, 11753412
benchmarks/2.9.0 (11.542M) : 0, 11542126
section Automatic
This PR (5739) (8.187M) : 0, 8187178
master (7.853M) : 0, 7853279
benchmarks/2.9.0 (8.263M) : 0, 8262905
section Trace stats
master (8.259M) : 0, 8258966
section Manual
This PR (5739) (10.470M) : 0, 10469997
master (9.899M) : 0, 9899401
section Manual + Automatic
This PR (5739) (7.672M) : 0, 7672159
master (7.433M) : 0, 7432897
section Version Conflict
master (6.731M) : 0, 6730574
gantt
title Throughput Linux arm64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (5739) (9.545M) : 0, 9545011
master (9.574M) : 0, 9574121
benchmarks/2.9.0 (9.596M) : 0, 9596140
section Automatic
This PR (5739) (6.558M) : 0, 6558237
master (6.671M) : 0, 6671192
section Trace stats
master (6.990M) : 0, 6990244
section Manual
This PR (5739) (8.382M) : 0, 8381864
master (8.307M) : 0, 8306500
section Manual + Automatic
This PR (5739) (6.028M) : 0, 6027873
master (6.251M) : 0, 6250704
section Version Conflict
master (5.762M) : 0, 5761603
gantt
title Throughput Windows x64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (5739) (10.009M) : 0, 10008907
master (10.202M) : 0, 10201759
benchmarks/2.9.0 (10.213M) : 0, 10213239
section Automatic
This PR (5739) (7.087M) : 0, 7086987
master (7.212M) : 0, 7212394
benchmarks/2.9.0 (7.482M) : 0, 7482023
section Trace stats
master (7.549M) : 0, 7549360
section Manual
This PR (5739) (8.691M) : 0, 8691212
master (9.013M) : 0, 9012822
section Manual + Automatic
This PR (5739) (6.783M) : 0, 6783486
master (7.018M) : 0, 7017920
section Version Conflict
master (6.365M) : 0, 6365432
Benchmarks for #5739 compared to master:
The following thresholds were used for comparing the benchmark speeds:
Allocation changes below 0.5% are ignored.
LGTM, was only wondering how often this could happen that would make it worth adding some logging if so.
From the initial discovery/fix of this in #5690 we only saw a few instance of the actual exception in error tracking, so it may be relatively rare.
I don't think that logging would be that beneficial here as I think we understand the cause of it now and have some future action items to look into with how we could improve this bit of code and how exactly this would work with non-W3C IDs.
Summary of changes
This makes it so that we validate that the
Activity
object is in W3C format (by checking that theActivity
has a SpanID and TraceID after it stars) if we are updating the SpanId and TraceId.Note that for this to happen the
Activity
needs to be a non-W3C format and it's start time must be less than the Datadog Active Span start time. This start time issue can happen as anActivity
can modify its start time after it is started and we don't update the created Datadog span with that new information.Reason for change
T fixes an edge case where an
Activity
object could be in a non-W3C format and hit this code resulting in itsId
being erroneously set tonull
causing aNullReferenceException
for us (workaround in #5690), but could have also thrown if anyone accessed thatId
and attempted to make a call on it.Implementation details
Check to ensure that the
Activity
is in W3C format - this is done by just checking that it already has a Span Id and Trace Id.Test coverage
Added tests to
Samples.NetActivitySdk
, theRunManuallyUpdatedStartTime()
would fail and throw an exception.Other details
Should consider handling instances where users update the start time of the
Activity
and update the DatadogSpan
accordingly.