Automatically instrument methods decorated with [Trace] (Datadog.Trace.Annotations.TraceAttribute)#2606
Conversation
zacharycmontoya
added
the
area:native-library
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Haven't done a review yet, but I'm wondering what can be the impact on startup time? Could it be significant enough so that we should add a flag to allow the feature to be enabled (or disabled if we want to make it easier to use) by a customer? |
lucaspimentel
changed the title
[Trace] (Datadog.Trace.TraceAttribute)
zacharycmontoya
changed the title
[Trace] (Datadog.Trace.TraceAttribute)[Trace] (Datadog.Trace.Annotations.TraceAttribute)
9a93e1d to
941cb6c
Compare
andrewlock
left a comment
andrewlock
left a comment
There was a problem hiding this comment.
Nice! I have a couple of questions and comments, but we should also double check the nupkg is included in the build release artifacts. It should be, but worth checking if you haven't already 🙂
| /// Attribute that marks the decorated method to be instrumented | ||
| /// by Datadog automatic instrumentation. | ||
| /// </summary> | ||
| [AttributeUsage(AttributeTargets.Method, AllowMultiple = false)] |
There was a problem hiding this comment.
Can we use this on properties too, or only methods right now?
There was a problem hiding this comment.
Right now it's only methods, though I'm not sure how well the attribute would map to properties because a property can map to both a getter method and a setter method, and I'm not sure we'd want to set the same ResourceName for both. I just realized that I can decorate a property accessor method like the following (and added this to my integration tests in 46ae5f8dd067fd13db2b590f9a283f7ed6c1edea), so I think this will handle the case where users want to decorate property usages. Does that sound good?
class TestType
{
public string Name
{
[Trace]
get;
[Trace]
set;
}
}
There was a problem hiding this comment.
If we allow this:
class TestType
{
[Trace]
public string Name { get; set; }
}Wouldn't the resource names be get_Name and set_Name, since those are the actual method names behind-the-scenes? Hmm, but I guess that gets weird is the user overrides the resource name in the attribute.
There was a problem hiding this comment.
Yeah that's what I was trying to avoid, if the user decided to set a custom resource name but places the attribute on the property. That's why I'm proposing we limit the attribute usage to just methods instead of trying to make it work for properties. Plus property support on the native side would be more complicated 😅
| const void* attribute_data = nullptr; // We'll likely need to use this Hopefully we don't need to | ||
| // use this because I don't know how |
There was a problem hiding this comment.
😆 I'll clean this up now that it's implemented 😛
|
|
||
| <ItemGroup> | ||
| <ProjectReference Include="..\..\..\..\src\Datadog.Trace.Annotations\Datadog.Trace.Annotations.csproj"> | ||
| <Aliases>OfficialDatadogAlias</Aliases> |
There was a problem hiding this comment.
Me too, I discovered it when I wanted to only use one test application to test using the official type and a custom-made one 😆
| @@ -0,0 +1,13 @@ | |||
| # Datadog.Trace.Annotations NuGet package | |||
There was a problem hiding this comment.
Should we also add a note to the Datadog.Trace readme, mentioning the existence of this package?
There was a problem hiding this comment.
Good idea, I'll draft up some changes that you can review
This comment has been minimized.
This comment has been minimized.
|
|
||
| > Note: Automatic instrumentation is required for the attributes in this package to take effect. Please [read our documentation](https://docs.datadoghq.com/tracing/setup/dotnet) for details on how to install the tracer for automatic instrumentation. | ||
|
|
||
| > Note: If you are unable to add new package references to your application, you may still enable this functionality by defining types inside your application whose full name and type members match the definitions in this package. |
There was a problem hiding this comment.
Could we provide the minimal code for the attribute so users can copy/paste? Maybe right here in the readme, or link to tracer/src/Datadog.Trace.Annotations/TraceAttribute.cs in the repo.
There was a problem hiding this comment.
In the list of attributes below I provided a link to the repo, but I think we could also add the code snippet since this package is pretty small. I'll go ahead and add that
@pierotibou I added an optimization in 35e2222 to skip the new code path for all assemblies whose name starts with |
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.
…aceAnnotationsIntegration spans by decorating a method specified by DD_TRACE_METHODS with a Datadog.Trace.TraceAttribute Next: Have the profiler automatically find methods with the Datadog.Trace.TraceAttribute and instrument them with the TraceAnnotationsIntegration
…DD_TRACE_METHODS, decorate it with the Datadog.Trace.TraceAttribute, and assert that we have a new span. Also apply the attribute to methods already specified by DD_TRACE_METHODS and assert that we have the same number of spans and that the attribute's ResourceName and OperationName properties get applied to the generated span. Product: - Modify the ModuleLoadFinished callback to look through a module (so long as we don't intentionally skip it) to look for methods decorated with Datadog.Trace.TraceAttribute and request ReJIT on them. If the integrations have not been loaded yet, they're put on a queue to be requested for ReJIT at a later ModuleLoadFinished callback. - Add a new PInvoke in the profiler to add the TraceAnnotationsIntegration type earlier in the startup code path than the InitializeTraceMethods PInvoke, which occurs after Tracer initialization
…METHODS and mark it with [Trace] to demonstrate that we can instrument early methods (except for the entrypoint Main) at this point
…pt a TypeReference argument so we can re-use the one trace_annotation_integration_type TypeReference
…lection, do not depend on having a type reference to it
…aceAttribute and move it to a new Datadog.Trace.Annotations assembly/package - In Samples.TraceAnnotations, test the official Datadog.Trace.Annotations.TraceAttribute type from the Datadog.Trace.Annotations assembly under alias OfficialTraceAttribute and test a custom-built Datadog.Trace.Annotations.TraceAttribute type from the Samples.TraceAnnotations assembly under alias CustomTraceAttribute
… some small refactorin
Co-authored-by: Andrew Lock <[email protected]>
… this to developers
- Apply nullable at project level for Datadog.Trace.Annotations
- Add attribute to property setter to demonstrate how to use the trace attribute on property accessors methods
- Create a constant field for WStr("Datadog.Trace.Annotations.TraceAttribute") and a static field for its c_string
- Fix comments
- Fix public API snapshots for net6.0
… running multiple tests at once
…Annotations NuGet package from the Datadog.Trace README. Also add further documentation in the Datadog.Trace.Annotations README.
…ave the following prefixes - "System." - "Microsoft." - "Datadog."
…raceAnnotations.VersionMismatch.NewerNuGet test application that contains the logic to scan for [Trace]
913b0dd to
d5ceab4
Compare
This comment has been minimized.
This comment has been minimized.
…trace annotations feature is enabled, since it incurs an added runtime cost. The default is true so users can immediately take advantage of the feature
Just to be on the safe side, I've added a feature flag in a5ffc98 titled |
Code Coverage Report 📊✔️ Merging #2606 into master will not change line coverage
View the full report for further details: Datadog.Trace Breakdown ✔️
The following classes have significant coverage changes.
View the full reports for further details: |
Benchmarks Report 🐌Benchmarks for #2606 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 - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.DbCommandBenchmark - Slower
|
| Benchmark | diff/base | Base Median (ns) | Diff Median (ns) | Modality |
|---|---|---|---|---|
| Benchmarks.Trace.DbCommandBenchmark.ExecuteNonQuery‑net472 | 1.327 | 1,760.72 | 2,335.94 | bimodal |
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | ExecuteNonQuery |
net472 | 1.76μs | 8.4ns | 32.5ns | 0.0937 | 0 | 0 | 594 B |
| master | ExecuteNonQuery |
netcoreapp3.1 | 1.51μs | 7.25ns | 28.1ns | 0.00903 | 0 | 0 | 632 B |
| #2606 | ExecuteNonQuery |
net472 | 2.34μs | 13.7ns | 124ns | 0.094 | 0.00116 | 0 | 594 B |
| #2606 | ExecuteNonQuery |
netcoreapp3.1 | 1.47μs | 4.71ns | 18.2ns | 0.00873 | 0 | 0 | 632 B |
Benchmarks.Trace.ElasticsearchBenchmark - Slower ⚠️ Same allocations ✔️
Slower ⚠️ in #2606
Benchmark
diff/base
Base Median (ns)
Diff Median (ns)
Modality
Benchmarks.Trace.ElasticsearchBenchmark.CallElasticsearch‑netcoreapp3.1
1.136
1,770.64
2,011.31
| Benchmark | diff/base | Base Median (ns) | Diff Median (ns) | Modality |
|---|---|---|---|---|
| Benchmarks.Trace.ElasticsearchBenchmark.CallElasticsearch‑netcoreapp3.1 | 1.136 | 1,770.64 | 2,011.31 |
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | CallElasticsearch |
net472 | 2.66μs | 13.4ns | 59.9ns | 0.126 | 0 | 0 | 803 B |
| master | CallElasticsearch |
netcoreapp3.1 | 1.78μs | 8.42ns | 34.7ns | 0.0115 | 0 | 0 | 792 B |
| master | CallElasticsearchAsync |
net472 | 3.21μs | 14.4ns | 55.6ns | 0.148 | 0.00156 | 0 | 939 B |
| master | CallElasticsearchAsync |
netcoreapp3.1 | 1.92μs | 10.3ns | 51.3ns | 0.0122 | 0 | 0 | 912 B |
| #2606 | CallElasticsearch |
net472 | 2.57μs | 13.7ns | 73.7ns | 0.127 | 0 | 0 | 803 B |
| #2606 | CallElasticsearch |
netcoreapp3.1 | 2.02μs | 11.5ns | 79.9ns | 0.0106 | 0 | 0 | 792 B |
| #2606 | CallElasticsearchAsync |
net472 | 3.06μs | 14.6ns | 61.8ns | 0.148 | 0.00151 | 0 | 939 B |
| #2606 | CallElasticsearchAsync |
netcoreapp3.1 | 1.91μs | 10.6ns | 67.1ns | 0.0128 | 0 | 0 | 912 B |
Benchmarks.Trace.GraphQLBenchmark - Slower ⚠️ Same allocations ✔️
Slower ⚠️ in #2606
Benchmark
diff/base
Base Median (ns)
Diff Median (ns)
Modality
Benchmarks.Trace.GraphQLBenchmark.ExecuteAsync‑netcoreapp3.1
1.249
2,001.94
2,500.80
| Benchmark | diff/base | Base Median (ns) | Diff Median (ns) | Modality |
|---|---|---|---|---|
| Benchmarks.Trace.GraphQLBenchmark.ExecuteAsync‑netcoreapp3.1 | 1.249 | 2,001.94 | 2,500.80 |
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | ExecuteAsync |
net472 | 2.92μs | 13.8ns | 55.2ns | 0.168 | 0 | 0 | 1.06 KB |
| master | ExecuteAsync |
netcoreapp3.1 | 2.01μs | 9.3ns | 36ns | 0.0141 | 0 | 0 | 1.03 KB |
| #2606 | ExecuteAsync |
net472 | 2.99μs | 15.3ns | 70ns | 0.168 | 0.00147 | 0 | 1.06 KB |
| #2606 | ExecuteAsync |
netcoreapp3.1 | 2.51μs | 9.16ns | 33ns | 0.0146 | 0 | 0 | 1.03 KB |
Benchmarks.Trace.HttpClientBenchmark - Faster 🎉 Same allocations ✔️
Faster 🎉 in #2606
Benchmark
base/diff
Base Median (ns)
Diff Median (ns)
Modality
Benchmarks.Trace.HttpClientBenchmark.SendAsync‑netcoreapp3.1
1.153
5,494.95
4,767.50
| Benchmark | base/diff | Base Median (ns) | Diff Median (ns) | Modality |
|---|---|---|---|---|
| Benchmarks.Trace.HttpClientBenchmark.SendAsync‑netcoreapp3.1 | 1.153 | 5,494.95 | 4,767.50 |
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | SendAsync |
net472 | 7.05μs | 37.8ns | 207ns | 0.361 | 0 | 0 | 2.28 KB |
| master | SendAsync |
netcoreapp3.1 | 5.47μs | 23.4ns | 90.6ns | 0.0328 | 0 | 0 | 2.21 KB |
| #2606 | SendAsync |
net472 | 6.67μs | 34.2ns | 153ns | 0.362 | 0 | 0 | 2.28 KB |
| #2606 | SendAsync |
netcoreapp3.1 | 4.78μs | 18.9ns | 73.3ns | 0.0311 | 0 | 0 | 2.21 KB |
Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | EnrichedLog |
net472 | 3.58μs | 18.1ns | 95.9ns | 0.227 | 0 | 0 | 1.45 KB |
| master | EnrichedLog |
netcoreapp3.1 | 3.25μs | 16.7ns | 80.1ns | 0.0207 | 0 | 0 | 1.53 KB |
| #2606 | EnrichedLog |
net472 | 3.29μs | 14.6ns | 54.5ns | 0.228 | 0 | 0 | 1.45 KB |
| #2606 | EnrichedLog |
netcoreapp3.1 | 3.09μs | 15.1ns | 62.2ns | 0.0213 | 0 | 0 | 1.53 KB |
Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | EnrichedLog |
net472 | 289μs | 1.04μs | 4.42μs | 0.434 | 0.145 | 0 | 4.33 KB |
| master | EnrichedLog |
netcoreapp3.1 | 226μs | 988ns | 3.7μs | 0 | 0 | 0 | 4.21 KB |
| #2606 | EnrichedLog |
net472 | 287μs | 1.41μs | 5.98μs | 0.416 | 0.139 | 0 | 4.33 KB |
| #2606 | EnrichedLog |
netcoreapp3.1 | 230μs | 1.14μs | 4.72μs | 0 | 0 | 0 | 4.21 KB |
Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | EnrichedLog |
net472 | 7.45μs | 30.7ns | 119ns | 0.504 | 0 | 0 | 3.23 KB |
| master | EnrichedLog |
netcoreapp3.1 | 6.62μs | 31.4ns | 122ns | 0.0513 | 0 | 0 | 3.6 KB |
| #2606 | EnrichedLog |
net472 | 7.55μs | 36.2ns | 140ns | 0.504 | 0 | 0 | 3.23 KB |
| #2606 | EnrichedLog |
netcoreapp3.1 | 6.84μs | 38.6ns | 273ns | 0.0517 | 0 | 0 | 3.6 KB |
Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | SendReceive |
net472 | 2.54μs | 13.8ns | 79.5ns | 0.16 | 0.00124 | 0 | 1.01 KB |
| master | SendReceive |
netcoreapp3.1 | 2.23μs | 11.6ns | 59ns | 0.0135 | 0 | 0 | 1.01 KB |
| #2606 | SendReceive |
net472 | 2.49μs | 13.9ns | 90ns | 0.158 | 0 | 0 | 1.01 KB |
| #2606 | SendReceive |
netcoreapp3.1 | 2.03μs | 6.06ns | 21.9ns | 0.0147 | 0 | 0 | 1.01 KB |
Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | EnrichedLog |
net472 | 5.93μs | 28.7ns | 118ns | 0.291 | 0 | 0 | 1.87 KB |
| master | EnrichedLog |
netcoreapp3.1 | 5.17μs | 24.1ns | 93.4ns | 0.0204 | 0 | 0 | 1.49 KB |
| #2606 | EnrichedLog |
net472 | 6.06μs | 28ns | 140ns | 0.292 | 0 | 0 | 1.87 KB |
| #2606 | EnrichedLog |
netcoreapp3.1 | 5.26μs | 19.5ns | 75.7ns | 0.0208 | 0 | 0 | 1.49 KB |
Benchmarks.Trace.SpanBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | StartFinishSpan |
net472 | 905ns | 3.48ns | 13ns | 0.0719 | 0 | 0 | 457 B |
| master | StartFinishSpan |
netcoreapp3.1 | 899ns | 3.69ns | 14.3ns | 0.00635 | 0 | 0 | 456 B |
| master | StartFinishScope |
net472 | 1.1μs | 5.51ns | 23.4ns | 0.0842 | 0 | 0 | 538 B |
| master | StartFinishScope |
netcoreapp3.1 | 1.13μs | 6.46ns | 45.2ns | 0.00778 | 0 | 0 | 576 B |
| #2606 | StartFinishSpan |
net472 | 906ns | 4.34ns | 17.4ns | 0.0716 | 0 | 0 | 457 B |
| #2606 | StartFinishSpan |
netcoreapp3.1 | 891ns | 4.28ns | 16.6ns | 0.00623 | 0 | 0 | 456 B |
| #2606 | StartFinishScope |
net472 | 1.2μs | 6.65ns | 40.4ns | 0.0845 | 0 | 0 | 538 B |
| #2606 | StartFinishScope |
netcoreapp3.1 | 1.04μs | 4.14ns | 16ns | 0.00761 | 0 | 0 | 576 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 |
net472 | 1.31μs | 6.4ns | 27.1ns | 0.0839 | 0 | 0 | 538 B |
| master | RunOnMethodBegin |
netcoreapp3.1 | 1.24μs | 6.61ns | 35.6ns | 0.00784 | 0 | 0 | 576 B |
| #2606 | RunOnMethodBegin |
net472 | 1.25μs | 6.47ns | 29.6ns | 0.084 | 0 | 0 | 538 B |
| #2606 | RunOnMethodBegin |
netcoreapp3.1 | 1.19μs | 5.16ns | 20ns | 0.00826 | 0 | 0 | 576 B |
5 participants
Summary of changes
Adds a new public type named
Datadog.Trace.Annotations.TraceAttribute. Application code can add the attribute to any method with[Trace]and automatic instrumentation will automatically instrument it at runtime, with the following span metadata.To avoid having applications depend on
Datadog.Trace.dllfor this attribute, define it in a new assemblyDatadog.Trace.Annotations.dll.Span metadata
Operation Name:
TraceAttribute.OperationName ?? "trace.annotation"Resource Name:
TraceAttribute.ResourceName ?? "<METHOD_NAME>"Tags:
Configuration
This feature is enabled by default. If the overhead is too much and users are not using the feature, it can be disabled by setting
DD_TRACE_ANNOTATIONS_ENABLED=false.Implementation details
AddTraceAttributeInstrumentationwhich is invoked early (like the ByRef instrumentation) before the staticTracer.Instanceis initialized. This initializes theTraceAnnotationsIntegrationused by both[Trace]instrumentation andDD_TRACE_METHODSinstrumentation.Datadog.Trace.Annotations.TraceAttribute. If theTraceAnnotationsIntegrationhas been initialized, immediately request a ReJIT for each method with integration_type=TraceAnnotationsIntegration. If the integration is not yet initialized, put all the found methods on a queue to be processed in a later ModuleLoadFinished callback.TraceAnnotationsIntegrationhas been initialized, and if it is then empty the queue and request a ReJIT for each method.Test coverage
Update Samples.TraceAnnotations test application and its integration test in the following ways:
Samples.TraceAnnotations.Program[RunTestsAsync]from theDD_TRACE_METHODSconfiguration and place the[Trace]attribute on the method and assert that the span is still generated[Trace]attributes onto existing methods specified byDD_TRACE_METHODSand assert that the spans have the same OperationName/ResourceName as the attribute[Trace]attribute and are not specified inDD_TRACE_METHODS. Assert that new spans are created for the new methodsDatadog.Trace.Annotatations.TraceAttributetype and a user-definedDatadog.Trace.Annotations.TraceAttributeto ensure that both result in instrumentation