Activity Compatibility Layer #2446
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
In principle, I think this looks really nice 😍 I'd like to see us testing more approaches to creating activities (I know this is just a POC, just not sure if it will reveal any issues) e.g.: var activity = new Activity("Logging Activity");
try
{
activity.Start();
activity.AddTag("cart.operation.id", 123);
}
finally
{
activity.Stop();
}Though I think we may need to think about the "ignore list". 🤔 For example, in an ASP.NET Core app, the framework creates the activity for the request, and you typically add high cardinality tags to that "root" activity. Users will likely want those to be applied to our root ASP.NET Core span, which is, difficult 😕 It means we would need to map some of the activities to our existing spans (if they're in the trace), to make sure we are adding the tags to the right span (This recent post by Jimmy Bogard describes what I'm talking about) Unless, of course, we completely replace our ASP.NET Core diagnostic source implementation with your Activity approach (where possible) 🤔 That may not be feasible, I'm not sure. Also, it looks like there's a risk of "leaking" spans, if the customer doesn't stop the activity? Obviously that's an issue in their code, but it's something we probably want/need to handle somehow. I haven't got a clue how we do that and ensure we maintain the hierarchy correctly though, so maybe we can't resolve that one😟 |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
LGTM, amazing work as always 🎉
One part of the design that I think is worth mentioning in the PR description is how we handle a new "root" Activity object when there iss already an active Datadog Span: In this case, activity.Parent == null (there's no in-process parent Activity object) but we set the ParentId property to the parent Span's SpanId
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
| Assert.Contains(spans, span => span.Name == expectedOperationName); | ||
| Assert.Contains(spans, span => span.Name == spanFromActivityOperationName); |
There was a problem hiding this comment.
I'd definitely prefer we use snapshots here, to make sure all the parents etc line up.
There was a problem hiding this comment.
well both spans doesn't share the same traceId.... They are not part of the same execution context, and we are not testing the context propagation. (Also, we don't have an API to set the context of an activity yet), only W3C is supported automatically. That's the reason I thought this was enough.
There was a problem hiding this comment.
They are not part of the same execution context, and we are not testing the context propagation. (Also, we don't have an API to set the context of an activity yet), only W3C is supported automatically
Hmmm, I feel like we should have integration tests for that 🤔 Definitely happy for that to be a new PR though!
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Benchmarks Report 🐌Benchmarks for #2446 compared to master:
The following thresholds were used for comparing the benchmark speeds:
Allocation changes below 0.5% are ignored. Benchmark detailsBenchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AppSecBodyBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AspNetCoreBenchmark - Unknown 🤷 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 - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.TraceAnnotationsBenchmark - Same speed ✔️ Same allocations ✔️Raw results
|
Code Coverage Report 📊
View the full report for further details: Datadog.Trace Breakdown
|
| master | #2446 | Change | |
|---|---|---|---|
| Lines % | 71% |
71% |
-1% |
| Branches % | 68% |
67% |
-2% |
| Complexity | 12456 |
12999 |
543 ⛔ |
The following classes have significant coverage changes.
| File | Line coverage change | Branch coverage change | Complexity change |
|---|---|---|---|
| Datadog.Trace.Agent.Transports.ApiWebRequestFactory | -28% ⛔ |
-38% ⛔ |
12 ⛔ |
| Datadog.Trace.Telemetry.JsonWebRequestTelemetryTransport | -27% ⛔ |
-57% ⛔ |
0 ✔️ |
| Datadog.Trace.Telemetry.JsonHttpClientTelemetryTransport | -26% ⛔ |
-38% ⛔ |
0 ✔️ |
| Datadog.Trace.SpanContext | -25% ⛔ |
-34% ⛔ |
74 ⛔ |
| Datadog.Trace.Logging.DirectSubmission.Sink.LogsApi | -25% ⛔ |
-29% ⛔ |
33 ⛔ |
| Datadog.Trace.Configuration.ImmutableTracerSettings | -17% ⛔ |
-32% ⛔ |
21 ⛔ |
| Datadog.Trace.TracerManager | -17% ⛔ |
-30% ⛔ |
66 ⛔ |
| Datadog.Trace.Configuration.TracerSettings | -11% ⛔ |
-7% ⛔ |
39 ⛔ |
| Datadog.Trace.Telemetry.ConfigurationTelemetryCollector | -10% ⛔ |
-19% ⛔ |
4 ⛔ |
| Datadog.Trace.Tracer | -10% ⛔ |
-12% ⛔ |
18 ⛔ |
| ...And 5 more |
The following classes were added in #2446:
| File | Line coverage | Branch coverage | Complexity |
|---|---|---|---|
| Datadog.Trace.Activity.ActivityListener | 79% |
52% |
48 |
| Datadog.Trace.Activity.ActivityListenerDelegatesBuilder | 100% |
75% |
11 |
| Datadog.Trace.Activity.ActivityListenerHandler | 72% |
68% |
27 |
| Datadog.Trace.Activity.DiagnosticObserverListener | 75% |
67% |
7 |
| Datadog.Trace.Activity.DiagnosticSourceEventListener | 70% |
88% |
27 |
| ...And 3 more |
View the full reports for further details:
This PR enables support for
System.Diagnostics.Activityclass behindDD_TRACE_ACTIVITY_LISTENER_ENABLEDexperimental feature flag (here).The implementation creates a Datadog
Spanobject for each activity, and handles interoperation between activities and span objects keeping theActiveScopeupdated. (See Tests)Diagram
By using W3C propagators, we can interoperate with
OpenTelemetryapplications and keep the same distributed trace.Support
The listener supports
System.Diagnostics.DiagnosticSourceassembly from dll version4.0.4(Nuget version4.6.0) and tested up to version 6.Note: we are using DuckTyping to interact with the library, no vendoring or strong reference is required.
Handlers
The PR contains a standard way to create new handlers and change the behavior of the library depending of the
DiagnosticSourceName. Currently there are two implementations:IgnoreActivityHandler:a Handler to ignore activities created from the following sources:Couchbase.DotnetSdk.RequestTracerHttpHandlerDiagnosticListenerMassTransitMicrosoft.AspNetCoreMicrosoft.EntityFrameworkCoreMySqlConnectorNpgsqlSystem.Net.Http.DesktopSqlClientDiagnosticListenerThe reason we are ignoring these sources is because we already have a working integration that handles those spans.
DefaultActivityHandler:Algorithm explained in the diagram for any other source.Example