-
Notifications
You must be signed in to change notification settings - Fork 157
feat: Stable Session ID headers for telemetry #8352
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 5 commits
dc79c61
d47e797
2804636
4351aa0
642b76b
d7ec963
6385c47
3406b39
9eff4f3
8f98088
7782691
38678c1
56e6aea
98cca06
604f9ba
64d70b5
4f3274a
fbb50f6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1580,6 +1580,15 @@ supportedConfigurations: | |
| documentation: |- | ||
| Configuration key for whether telemetry metrics should be sent. | ||
| <see cref="Datadog.Trace.Telemetry.TelemetrySettings.MetricsEnabled"/> | ||
| _DD_ROOT_DOTNET_SESSION_ID: | ||
| - implementation: A | ||
| type: string | ||
| default: null | ||
| product: Telemetry | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this needs to be marked as a "platform key" so that it can be read directly from the environment
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately, anything starting with
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any resolution on this @anna-git @andrewlock? Since this is purely an internal implementation detail (the RFC only mandates the HTTP header names, not the env var), I'm happy to rename it to whatever works best with DOTNET standards whether that's dropping the _DD prefix to allow it to be a platform key, or keeping the current approach
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resolved per discussion with @anna-git , this should stay as a config key in ConfigRegistry (not a platform key). Platform keys are for non-Datadog config keys. Since this is ours ( |
||
| const_name: RootSessionId | ||
| documentation: |- | ||
| Internal env var for propagating the root session ID to child processes. | ||
| Set automatically by the tracer at init time; not user-configurable. | ||
| DD_TEST_MANAGEMENT_ATTEMPT_TO_FIX_RETRIES: | ||
| - implementation: A | ||
| type: int | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,7 @@ | |
| using Datadog.Trace.Logging; | ||
| using Datadog.Trace.PlatformHelpers; | ||
| using Datadog.Trace.Telemetry.Metrics; | ||
| using Datadog.Trace.Util; | ||
| using Datadog.Trace.Util.Http; | ||
| using Datadog.Trace.Vendors.Newtonsoft.Json; | ||
| using Datadog.Trace.Vendors.Newtonsoft.Json.Serialization; | ||
|
|
@@ -56,6 +57,14 @@ public async Task<TelemetryPushResult> PushTelemetry(TelemetryData data) | |
| request.AddHeader(TelemetryConstants.DebugHeader, "true"); | ||
| } | ||
|
|
||
| var sessionId = RuntimeId.Get(); | ||
| request.AddHeader(TelemetryConstants.SessionIdHeader, sessionId); | ||
| var rootSessionId = RuntimeId.GetRootSessionId(); | ||
khanayan123 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| if (rootSessionId != sessionId) | ||
| { | ||
| request.AddHeader(TelemetryConstants.RootSessionIdHeader, rootSessionId); | ||
| } | ||
|
||
|
|
||
| request.AddContainerMetadataHeaders(_containerMetadata); | ||
|
|
||
| TelemetryFactory.Metrics.RecordCountTelemetryApiRequests(endpointMetricTag); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -113,6 +113,11 @@ public TracerManager( | |
| ServiceRemappingHash = serviceRemappingHash; | ||
|
|
||
| SpanContextPropagator = SpanContextPropagatorFactory.GetSpanContextPropagator(settings.PropagationStyleInject, settings.PropagationStyleExtract, settings.PropagationExtractFirstOnly, settings.PropagationBehaviorExtract); | ||
|
|
||
| // Eagerly initialize the root session ID so child processes inherit it | ||
| // even if spawned before the first telemetry flush. | ||
| _ = RuntimeId.GetRootSessionId(); | ||
|
|
||
|
||
| UpdatePerTraceSettings(settings.Manager.InitialMutableSettings); | ||
| _settingSubscription = settings.Manager.SubscribeToChanges(changes => | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |
|
|
||
| using System; | ||
| using System.Threading; | ||
| using Datadog.Trace.Configuration; | ||
| using Datadog.Trace.Logging; | ||
|
|
||
| namespace Datadog.Trace.Util | ||
|
|
@@ -13,10 +14,13 @@ internal static class RuntimeId | |
| { | ||
| private static readonly IDatadogLogger Log = DatadogLogging.GetLoggerFor(typeof(RuntimeId)); | ||
| private static string _runtimeId; | ||
| private static string _rootSessionId; | ||
|
|
||
| public static string Get() => LazyInitializer.EnsureInitialized(ref _runtimeId, () => GetImpl()); | ||
| public static string Get() => LazyInitializer.EnsureInitialized(ref _runtimeId, () => GetRuntimeIdImpl()); | ||
|
||
|
|
||
| private static string GetImpl() | ||
| public static string GetRootSessionId() => LazyInitializer.EnsureInitialized(ref _rootSessionId, () => GetRootSessionIdImpl()); | ||
|
|
||
| private static string GetRuntimeIdImpl() | ||
| { | ||
| if (NativeLoader.TryGetRuntimeIdFromNative(out var runtimeId)) | ||
| { | ||
|
|
@@ -29,5 +33,19 @@ private static string GetImpl() | |
|
|
||
| return guid; | ||
| } | ||
|
|
||
| private static string GetRootSessionIdImpl() | ||
| { | ||
| var inherited = EnvironmentHelpers.GetEnvironmentVariable(ConfigurationKeys.Telemetry.RootSessionId); | ||
|
||
| if (!string.IsNullOrEmpty(inherited)) | ||
| { | ||
| Log.Debug("Inherited root session ID from parent: {RootSessionId}", inherited); | ||
| return inherited; | ||
| } | ||
|
|
||
| var rootId = Get(); | ||
| EnvironmentHelpers.SetEnvironmentVariable(ConfigurationKeys.Telemetry.RootSessionId, rootId); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For clarity, this simply won't work in many cases, such that this whole approach is potentially flawed...
The "good" news is that starting new dotnet processes is not a standard pattern in general, so hopefully it just doesn't really matter...
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as we already instrument
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yet we have a whole instrumentation for it. 😉 (edit: I had not refreshed to see Anna's comment before I posted mine 😅 )
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @lucaspimentel @anna-git @andrewlock Good point about the Process instrumentation. Would you prefer we inject _DD_ROOT_DOTNET_SESSION_ID directly into the child process's ProcessStartInfo.Environment via the existing ClrProfiler/AutoInstrumentation/Process hooks, rather than relying on Environment.SetEnvironmentVariable()? And would that address the Unix limitations Andrew mentioned and handle cases where the user provides a custom env block or sets UseShellExecute=true?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think since dotnet already instruments process start it might make sense to utilize that instead of env vars, we implemented it in a similar way for Node
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resolved per @andrewlock's recommendation, we'll keep
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
FWIW, I consider this to be very risky for breaking customers. I just don't see the value here as being worth the trade-off tbh |
||
| return rootId; | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -561,6 +561,21 @@ public void RuntimeId() | |
| Guid.TryParse(runtimeId, out _).Should().BeTrue(); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void RootSessionId_DefaultsToRuntimeId() | ||
| { | ||
| var rootSessionId = Datadog.Trace.Util.RuntimeId.GetRootSessionId(); | ||
| rootSessionId.Should().Be(Datadog.Trace.Util.RuntimeId.Get()); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void RootSessionId_SetsEnvVar() | ||
| { | ||
| var rootSessionId = Datadog.Trace.Util.RuntimeId.GetRootSessionId(); | ||
| Environment.GetEnvironmentVariable(ConfigurationKeys.Telemetry.RootSessionId) | ||
| .Should().Be(rootSessionId); | ||
| } | ||
|
||
|
|
||
|
||
| [Fact] | ||
| public async Task ForceFlush() | ||
| { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the leading underscore?
_DD...instead ofDD..? Is that something we're doing now for internal env vars? The RFC saysDD_ROOT_<LIB>_SESSION_ID.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, this seems non standard
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefixing environment variables with
_DDis convention we use in dd-trace-py to mark a configuration as internal/private. All implementations currently follow this spec, the implementation plan was out of date but I just pushed a fix.Ideally we'd use
_DDto be consistent across SDKs but this isn't a big deal for us. We would like to do what's best for the .NET library and ideally this PR would introduce minimal changes to library internals. From a backend perspective as long as the expected headers sent by the telemetry client we should be good.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that's the standard and it follows the spec and the other implementations, then yeah, go for it. I only asked because I had not seen it before and I didn't see in the RFC. Thanks!
(I wonder though it we have places where we look for
DD_*env vars that would need to be updated. Probably not production code, but maybe tests or scripts that list all DD env vars for troubleshooting.)Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The _DD prefix was established by the Python implementation (dd-trace-py#16839).
The implementation proposal/parent RFC only mandates the HTTP header names (DD-Session-ID, DD-Root-Session-ID) the env var naming (DD_ROOT_LIB_SESSION_ID) is mentioned only in the "Process Spawning: Fork & Exec Support" appendix as an implementation detail.
The underscore prefix signals this is internal: we don't want to expose it as a public API, and keeping it private gives us flexibility to replace the propagation mechanism in a minor version without a breaking change.
That said, this is purely an internal convention if the DD_ prefix is preferred for the DOTNET implementation, I can update it. It's not customer-facing either way.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per discussion on slack and after updating the implementation proposal we will keep _DD_ROOT_LIB_SESSION_ID as the naming convention across SDKs for this env var