Skip to content

Commit 84056f1

Browse files
authored
Add [MethodImpl(MethodImplOptions.NoInlining] to all our instrumented APIs in Datadog.Trace.Manual (#7302)
## Summary of changes Adds `[MethodImpl(MethodImplOptions.NoInlining]` to avoid race conditions on app startup ## Reason for change There’s essentially a race condition with how the .NET runtime profiling APIs work and how we inject into a customer’s application. We typically inject into the start of the `Main()` method, and then request ReJIT for all the methods that we want to instrument. The next time a target method is called, the runtime will see the ReJIT request, and add our instrumentation. Another aspect is inlining; if a method that we wanted to instrument has been inlined into the caller, then just doing a ReJIT won't work (because the method is "gone" in this case). We work around that for normally by blocking inlining for methods that we’re interested in (i.e. methods we instrument). However, if the method we’re interested in has _already_ been inlined _before_ we are injected then we could miss calls at the start of the app. Essentially there's a race condition between: - Calls in `Main()` being JITed (and inlined). - Our startup hook being injected and processing the instrumentation list to block inlining for methods we care about. We currently can't do much about this for arbitrary target methods (we have some ideas, but that's a separate issue!) but for our manual APIs we know we _will_ want to mark them as not-inlined. So we can essentially pre-emptively mark these APIs with `[MethodImpl(MethodImplOptions.NoInlining]`, exculding these methods from the race condition (theoretically; it's technically up to the JIT if it chooses to honour the attributes IIRC) ## Implementation details - Add `[MethodImpl(MethodImplOptions.NoInlining]` to all the methods marked with `[Instrumented]` - This is a bit ugly for properties because they need to be applied to the getter/setter, not the property - For consistency, moved the `[Instrumented]` attribute there too ## Test coverage Updated the unit test to ensure that every instrumented method is explicitly marked as `NoInlining`. Confirmed that this catches issues if you miss one ## Other details <!-- Fixes #{issue} --> <!-- ⚠️ Note: where possible, please obtain 2 approvals prior to merging. Unless CODEOWNERS specifies otherwise, for external teams it is typically best to have one review from a team member, and one review from apm-dotnet. Trivial changes do not require 2 reviews. -->
1 parent 763b1ed commit 84056f1

20 files changed

+273
-73
lines changed

tracer/src/Datadog.Trace.Manual/AppSec/EventTrackingSdk.cs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55

66
#nullable enable
77

8+
using System.Runtime.CompilerServices;
89
using Datadog.Trace.SourceGenerators;
910

1011
namespace Datadog.Trace.AppSec;
@@ -19,6 +20,7 @@ public static class EventTrackingSdk
1920
/// </summary>
2021
/// <param name="userId">The userId associated with the login success</param>
2122
[Instrumented]
23+
[MethodImpl(MethodImplOptions.NoInlining)]
2224
public static void TrackUserLoginSuccessEvent(string userId)
2325
{
2426
}
@@ -29,6 +31,7 @@ public static void TrackUserLoginSuccessEvent(string userId)
2931
/// <param name="userId">The userId associated with the login success</param>
3032
/// <param name="metadata">Metadata associated with the login success</param>
3133
[Instrumented]
34+
[MethodImpl(MethodImplOptions.NoInlining)]
3235
public static void TrackUserLoginSuccessEvent(string userId, IDictionary<string, string> metadata)
3336
{
3437
}
@@ -39,6 +42,7 @@ public static void TrackUserLoginSuccessEvent(string userId, IDictionary<string,
3942
/// <param name="userId">The userId associated with the login failure</param>
4043
/// <param name="exists">If the userId associated with the login failure exists</param>
4144
[Instrumented]
45+
[MethodImpl(MethodImplOptions.NoInlining)]
4246
public static void TrackUserLoginFailureEvent(string userId, bool exists)
4347
{
4448
}
@@ -50,6 +54,7 @@ public static void TrackUserLoginFailureEvent(string userId, bool exists)
5054
/// <param name="exists">If the userId associated with the login failure exists</param>
5155
/// <param name="metadata">Metadata associated with the login failure</param>
5256
[Instrumented]
57+
[MethodImpl(MethodImplOptions.NoInlining)]
5358
public static void TrackUserLoginFailureEvent(string userId, bool exists, IDictionary<string, string> metadata)
5459
{
5560
}
@@ -59,6 +64,7 @@ public static void TrackUserLoginFailureEvent(string userId, bool exists, IDicti
5964
/// </summary>
6065
/// <param name="eventName">the name of the event to be tracked</param>
6166
[Instrumented]
67+
[MethodImpl(MethodImplOptions.NoInlining)]
6268
public static void TrackCustomEvent(string eventName)
6369
{
6470
}
@@ -69,6 +75,7 @@ public static void TrackCustomEvent(string eventName)
6975
/// <param name="eventName">the name of the event to be tracked</param>
7076
/// <param name="metadata">Metadata associated with the custom event</param>
7177
[Instrumented]
78+
[MethodImpl(MethodImplOptions.NoInlining)]
7279
public static void TrackCustomEvent(string eventName, IDictionary<string, string> metadata)
7380
{
7481
}

tracer/src/Datadog.Trace.Manual/AppSec/EventTrackingSdkV2.cs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
// </copyright>
55

66
#nullable enable
7+
using System.Runtime.CompilerServices;
78
using Datadog.Trace.SourceGenerators;
89

910
namespace Datadog.Trace.AppSec;
@@ -21,6 +22,7 @@ public static class EventTrackingSdkV2
2122
/// <param name="userId">The optional userId associated with the login success</param>
2223
/// <param name="metadata">The optional metadata associated with the login success</param>
2324
[Instrumented]
25+
[MethodImpl(MethodImplOptions.NoInlining)]
2426
public static void TrackUserLoginSuccess(string userLogin, string? userId = null, IDictionary<string, string>? metadata = null)
2527
{
2628
}
@@ -32,6 +34,7 @@ public static void TrackUserLoginSuccess(string userLogin, string? userId = null
3234
/// <param name="userDetails">The optional userDetails associated with the login success</param>
3335
/// <param name="metadata">The optional metadata associated with the login success</param>
3436
[Instrumented]
37+
[MethodImpl(MethodImplOptions.NoInlining)]
3538
public static void TrackUserLoginSuccess(string userLogin, UserDetails userDetails, IDictionary<string, string>? metadata = null)
3639
{
3740
}
@@ -44,6 +47,7 @@ public static void TrackUserLoginSuccess(string userLogin, UserDetails userDetai
4447
/// <param name="userId">The optional userId associated with the login success</param>
4548
/// <param name="metadata">Metadata associated with the login failure</param>
4649
[Instrumented]
50+
[MethodImpl(MethodImplOptions.NoInlining)]
4751
public static void TrackUserLoginFailure(string userLogin, bool exists, string? userId = null, IDictionary<string, string>? metadata = null)
4852
{
4953
}
@@ -56,6 +60,7 @@ public static void TrackUserLoginFailure(string userLogin, bool exists, string?
5660
/// <param name="userDetails">The details of the user associated with the login failure</param>
5761
/// <param name="metadata">Metadata associated with the login failure</param>
5862
[Instrumented]
63+
[MethodImpl(MethodImplOptions.NoInlining)]
5964
public static void TrackUserLoginFailure(string userLogin, bool exists, UserDetails userDetails, IDictionary<string, string>? metadata = null)
6065
{
6166
}

tracer/src/Datadog.Trace.Manual/Baggage.cs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
// </copyright>
55

66
using System.Collections;
7+
using System.Runtime.CompilerServices;
78
using Datadog.Trace.SourceGenerators;
89

910
namespace Datadog.Trace;
@@ -19,15 +20,18 @@ public static class Baggage
1920
/// <summary>
2021
/// Gets or sets the baggage collection for the current execution context.
2122
/// </summary>
22-
[Instrumented]
2323
public static IDictionary<string, string?> Current
2424
{
25+
[Instrumented]
26+
[MethodImpl(MethodImplOptions.NoInlining)]
2527
get
2628
{
2729
// auto-instrumentation will return Trace.Baggage.Current instead
2830
return _current ??= new Dictionary<string, string?>();
2931
}
3032

33+
[Instrumented]
34+
[MethodImpl(MethodImplOptions.NoInlining)]
3135
set
3236
{
3337
// auto-instrumentation will add:

tracer/src/Datadog.Trace.Manual/Ci/TestExtensions.cs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
// This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2017 Datadog, Inc.
44
// </copyright>
55

6+
using System.Runtime.CompilerServices;
67
using Datadog.Trace.SourceGenerators;
78

89
namespace Datadog.Trace.Ci;
@@ -18,6 +19,7 @@ public static class TestExtensions
1819
/// <param name="test">The <see cref="ITest"/> instance to augment</param>
1920
/// <param name="parameters">TestParameters instance</param>
2021
[Instrumented]
22+
[MethodImpl(MethodImplOptions.NoInlining)]
2123
public static void SetParameters(this ITest test, TestParameters parameters)
2224
{
2325
}
@@ -29,6 +31,7 @@ public static void SetParameters(this ITest test, TestParameters parameters)
2931
/// <param name="hostInfo">Host info</param>
3032
/// <param name="jobInfo">Job info</param>
3133
[Instrumented]
34+
[MethodImpl(MethodImplOptions.NoInlining)]
3235
public static void SetBenchmarkMetadata(this ITest test, in BenchmarkHostInfo hostInfo, in BenchmarkJobInfo jobInfo)
3336
{
3437
}
@@ -41,6 +44,7 @@ public static void SetBenchmarkMetadata(this ITest test, in BenchmarkHostInfo ho
4144
/// <param name="info">Measure info</param>
4245
/// <param name="statistics">Statistics values</param>
4346
[Instrumented]
47+
[MethodImpl(MethodImplOptions.NoInlining)]
4448
public static void AddBenchmarkData(this ITest test, BenchmarkMeasureType measureType, string info, in BenchmarkDiscreteStats statistics)
4549
{
4650
}

tracer/src/Datadog.Trace.Manual/Ci/TestModule.cs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
// </copyright>
55
#nullable enable
66

7+
using System.Runtime.CompilerServices;
78
using Datadog.Trace.Ci.Stubs;
89
using Datadog.Trace.SourceGenerators;
910

@@ -44,6 +45,7 @@ public static ITestModule Create(string name, string framework, string framework
4445
=> InternalCreate(name, framework, frameworkVersion, startDate);
4546

4647
[Instrumented]
48+
[MethodImpl(MethodImplOptions.NoInlining)]
4749
internal static ITestModule InternalCreate(string name, string? framework, string? frameworkVersion, DateTimeOffset? startDate)
4850
=> NullTestModule.Instance;
4951
}

tracer/src/Datadog.Trace.Manual/Ci/TestSession.cs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
// </copyright>
55
#nullable enable
66

7+
using System.Runtime.CompilerServices;
78
using Datadog.Trace.Ci.Stubs;
89
using Datadog.Trace.SourceGenerators;
910

@@ -65,6 +66,7 @@ public static ITestSession GetOrCreate(string command, string? workingDirectory,
6566
=> InternalGetOrCreate(command, workingDirectory, framework, startDate, propagateEnvironmentVariables);
6667

6768
[Instrumented]
69+
[MethodImpl(MethodImplOptions.NoInlining)]
6870
internal static ITestSession InternalGetOrCreate(string command, string? workingDirectory, string? framework, DateTimeOffset? startDate, bool propagateEnvironmentVariables = false)
6971
=> NullTestSession.Instance;
7072
}

tracer/src/Datadog.Trace.Manual/Configuration/GlobalSettings.cs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
// This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2017 Datadog, Inc.
44
// </copyright>
55

6+
using System.Runtime.CompilerServices;
67
using Datadog.Trace.SourceGenerators;
78

89
namespace Datadog.Trace.Configuration;
@@ -22,6 +23,7 @@ internal GlobalSettings()
2223
/// </summary>
2324
/// <param name="enabled">Whether debug is enabled.</param>
2425
[Instrumented]
26+
[MethodImpl(MethodImplOptions.NoInlining)]
2527
public static void SetDebugEnabled(bool enabled)
2628
{
2729
}

tracer/src/Datadog.Trace.Manual/Configuration/ImmutableIntegrationSettings.cs

Lines changed: 25 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55

66
#nullable enable
77

8+
using System.Runtime.CompilerServices;
89
using Datadog.Trace.SourceGenerators;
910

1011
namespace Datadog.Trace.Configuration;
@@ -30,27 +31,43 @@ internal ImmutableIntegrationSettings(string name)
3031
/// <summary>
3132
/// Gets the name of the integration. Used to retrieve integration-specific settings.
3233
/// </summary>
33-
[Instrumented]
34-
public string IntegrationName { get; }
34+
public string IntegrationName
35+
{
36+
[Instrumented]
37+
[MethodImpl(MethodImplOptions.NoInlining)]
38+
get;
39+
}
3540

3641
/// <summary>
3742
/// Gets a value indicating whether
3843
/// this integration is enabled.
3944
/// </summary>
40-
[Instrumented]
41-
public bool? Enabled { get; }
45+
public bool? Enabled
46+
{
47+
[Instrumented]
48+
[MethodImpl(MethodImplOptions.NoInlining)]
49+
get;
50+
}
4251

4352
/// <summary>
4453
/// Gets a value indicating whether
4554
/// Analytics are enabled for this integration.
4655
/// </summary>
47-
[Instrumented]
48-
public bool? AnalyticsEnabled { get; }
56+
public bool? AnalyticsEnabled
57+
{
58+
[Instrumented]
59+
[MethodImpl(MethodImplOptions.NoInlining)]
60+
get;
61+
}
4962

5063
/// <summary>
5164
/// Gets a value between 0 and 1 (inclusive)
5265
/// that determines the sampling rate for this integration.
5366
/// </summary>
54-
[Instrumented]
55-
public double AnalyticsSampleRate { get; }
67+
public double AnalyticsSampleRate
68+
{
69+
[Instrumented]
70+
[MethodImpl(MethodImplOptions.NoInlining)]
71+
get;
72+
}
5673
}

tracer/src/Datadog.Trace.Manual/Configuration/ImmutableIntegrationSettingsCollection.cs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55

66
#nullable enable
77

8+
using System.Runtime.CompilerServices;
89
using Datadog.Trace.SourceGenerators;
910

1011
namespace Datadog.Trace.Configuration;
@@ -26,9 +27,10 @@ internal ImmutableIntegrationSettingsCollection(Dictionary<string, ImmutableInte
2627
/// </summary>
2728
/// <param name="integrationName">The name of the integration.</param>
2829
/// <returns>The integration-specific settings for the specified integration.</returns>
29-
[Instrumented]
3030
public ImmutableIntegrationSettings this[string integrationName]
3131
{
32+
[Instrumented]
33+
[MethodImpl(MethodImplOptions.NoInlining)]
3234
get
3335
{
3436
if (Settings.TryGetValue(integrationName, out var setting))

0 commit comments

Comments
 (0)