-
Notifications
You must be signed in to change notification settings - Fork 155
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 all 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 |
|---|---|---|
| @@ -1,56 +1,103 @@ | ||
| // <copyright file="TelemetryHttpHeaderNames.cs" company="Datadog"> | ||
| // <copyright file="TelemetryHttpHeaderNames.cs" company="Datadog"> | ||
| // Unless explicitly stated otherwise all files in this repository are licensed under the Apache 2 License. | ||
| // This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2017 Datadog, Inc. | ||
| // </copyright> | ||
|
|
||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Threading; | ||
| using Datadog.Trace.HttpOverStreams; | ||
| using Datadog.Trace.Util; | ||
|
|
||
| namespace Datadog.Trace.Telemetry | ||
| { | ||
| internal static class TelemetryHttpHeaderNames | ||
| { | ||
| private static string _httpSerializedDefaultAgentHeaders; | ||
|
|
||
| /// <summary> | ||
| /// Returns <see cref="GetDefaultAgentHeaders"/>, in the format <c>Key: Value\r\n</c>. For use in HTTP headers | ||
| /// Gets the default agent headers in the format <c>Key: Value\r\n</c>. For use in HTTP headers. | ||
| /// Lazily initialized because session headers depend on runtime values. | ||
| /// </summary> | ||
| internal const string HttpSerializedDefaultAgentHeaders = | ||
| $"{TelemetryConstants.ClientLibraryLanguageHeader}: {TracerConstants.Language}" + DatadogHttpValues.CrLf + | ||
| $"{TelemetryConstants.ClientLibraryVersionHeader}: {TracerConstants.AssemblyVersion}" + DatadogHttpValues.CrLf + | ||
| $"{HttpHeaderNames.TracingEnabled}: false" + DatadogHttpValues.CrLf; | ||
| internal static string HttpSerializedDefaultAgentHeaders => | ||
| LazyInitializer.EnsureInitialized(ref _httpSerializedDefaultAgentHeaders, BuildSerializedAgentHeaders); | ||
|
|
||
| /// <summary> | ||
| /// Gets the default constant headers that should be added to any request to the agent | ||
| /// </summary> | ||
| internal static KeyValuePair<string, string>[] GetDefaultAgentHeaders() | ||
| => | ||
| [ | ||
| new(TelemetryConstants.ClientLibraryLanguageHeader, TracerConstants.Language), | ||
| new(TelemetryConstants.ClientLibraryVersionHeader, TracerConstants.AssemblyVersion), | ||
| new(HttpHeaderNames.TracingEnabled, "false") // don't add automatic instrumentation to requests directed to the agent | ||
| ]; | ||
| { | ||
| var sessionId = RuntimeId.Get(); | ||
| var rootSessionId = RuntimeId.GetRootSessionId(); | ||
| var includeRoot = rootSessionId != sessionId; | ||
| var headerCount = includeRoot ? 5 : 4; | ||
|
|
||
| var headers = new KeyValuePair<string, string>[headerCount]; | ||
| headers[0] = new(TelemetryConstants.ClientLibraryLanguageHeader, TracerConstants.Language); | ||
| headers[1] = new(TelemetryConstants.ClientLibraryVersionHeader, TracerConstants.AssemblyVersion); | ||
| headers[2] = new(HttpHeaderNames.TracingEnabled, "false"); // don't add automatic instrumentation to requests directed to the agent | ||
| headers[3] = new(TelemetryConstants.SessionIdHeader, sessionId); | ||
|
|
||
| if (includeRoot) | ||
| { | ||
| headers[4] = new(TelemetryConstants.RootSessionIdHeader, rootSessionId); | ||
| } | ||
|
|
||
| return headers; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets the default constant headers that should be added to any request to the direct telemetry intake | ||
| /// </summary> | ||
| internal static KeyValuePair<string, string>[] GetDefaultIntakeHeaders(TelemetrySettings.AgentlessSettings settings) | ||
| { | ||
| var headerCount = settings.Cloud is null ? 4 : 7; | ||
| var sessionId = RuntimeId.Get(); | ||
| var rootSessionId = RuntimeId.GetRootSessionId(); | ||
| var includeRoot = rootSessionId != sessionId; | ||
| var baseCount = settings.Cloud is null ? 5 : 8; | ||
| var headerCount = includeRoot ? baseCount + 1 : baseCount; | ||
|
|
||
| var headers = new KeyValuePair<string, string>[headerCount]; | ||
|
|
||
| headers[0] = new(TelemetryConstants.ClientLibraryLanguageHeader, TracerConstants.Language); | ||
| headers[1] = new(TelemetryConstants.ClientLibraryVersionHeader, TracerConstants.AssemblyVersion); | ||
| headers[2] = new(HttpHeaderNames.TracingEnabled, "false"); // don't add automatic instrumentation to requests directed to the agent | ||
| headers[2] = new(HttpHeaderNames.TracingEnabled, "false"); | ||
| headers[3] = new(TelemetryConstants.ApiKeyHeader, settings.ApiKey); | ||
| headers[4] = new(TelemetryConstants.SessionIdHeader, sessionId); | ||
|
|
||
| var index = 5; | ||
| if (settings.Cloud is { } cloud) | ||
| { | ||
| headers[4] = new(TelemetryConstants.CloudProviderHeader, cloud.Provider); | ||
| headers[5] = new(TelemetryConstants.CloudResourceTypeHeader, cloud.ResourceType); | ||
| headers[6] = new(TelemetryConstants.CloudResourceIdentifierHeader, cloud.ResourceIdentifier); | ||
| headers[index++] = new(TelemetryConstants.CloudProviderHeader, cloud.Provider); | ||
| headers[index++] = new(TelemetryConstants.CloudResourceTypeHeader, cloud.ResourceType); | ||
| headers[index++] = new(TelemetryConstants.CloudResourceIdentifierHeader, cloud.ResourceIdentifier); | ||
| } | ||
|
|
||
| if (includeRoot) | ||
| { | ||
| headers[index] = new(TelemetryConstants.RootSessionIdHeader, rootSessionId); | ||
| } | ||
|
|
||
| return headers; | ||
| } | ||
|
|
||
| private static string BuildSerializedAgentHeaders() | ||
| { | ||
| var sessionId = RuntimeId.Get(); | ||
|
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. There's another call to
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. Done |
||
| var rootSessionId = RuntimeId.GetRootSessionId(); | ||
|
|
||
| if (rootSessionId != sessionId) | ||
| { | ||
| return $"{TelemetryConstants.ClientLibraryLanguageHeader}: {TracerConstants.Language}" + DatadogHttpValues.CrLf + | ||
| $"{TelemetryConstants.ClientLibraryVersionHeader}: {TracerConstants.AssemblyVersion}" + DatadogHttpValues.CrLf + | ||
| $"{HttpHeaderNames.TracingEnabled}: false" + DatadogHttpValues.CrLf + | ||
| $"{TelemetryConstants.SessionIdHeader}: {sessionId}" + DatadogHttpValues.CrLf + | ||
| $"{TelemetryConstants.RootSessionIdHeader}: {rootSessionId}" + DatadogHttpValues.CrLf; | ||
| } | ||
|
|
||
| return $"{TelemetryConstants.ClientLibraryLanguageHeader}: {TracerConstants.Language}" + DatadogHttpValues.CrLf + | ||
| $"{TelemetryConstants.ClientLibraryVersionHeader}: {TracerConstants.AssemblyVersion}" + DatadogHttpValues.CrLf + | ||
| $"{HttpHeaderNames.TracingEnabled}: false" + DatadogHttpValues.CrLf + | ||
| $"{TelemetryConstants.SessionIdHeader}: {sessionId}" + DatadogHttpValues.CrLf; | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,17 +5,27 @@ | |
|
|
||
| using System; | ||
| using System.Threading; | ||
| using Datadog.Trace.Configuration; | ||
| using Datadog.Trace.Configuration.Telemetry; | ||
| using Datadog.Trace.Logging; | ||
| using Datadog.Trace.SourceGenerators; | ||
| using Datadog.Trace.Telemetry; | ||
|
|
||
| namespace Datadog.Trace.Util | ||
| { | ||
| 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 GetRootSessionId() => LazyInitializer.EnsureInitialized(ref _rootSessionId, () => GetRootSessionIdImpl()); | ||
|
|
||
| [TestingAndPrivateOnly] | ||
| internal static void ResetForTests() => _rootSessionId = null; | ||
khanayan123 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| private static string GetImpl() | ||
| { | ||
| if (NativeLoader.TryGetRuntimeIdFromNative(out var runtimeId)) | ||
|
|
@@ -29,5 +39,20 @@ private static string GetImpl() | |
|
|
||
| return guid; | ||
| } | ||
|
|
||
| private static string GetRootSessionIdImpl() | ||
| { | ||
| var config = new ConfigurationBuilder(new EnvironmentConfigurationSource(), TelemetryFactory.Config); | ||
| var inherited = config.WithKeys(ConfigurationKeys.Telemetry.RootSessionId).AsString(); | ||
| 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 |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| // <copyright file="RuntimeIdTests.cs" company="Datadog"> | ||
| // Unless explicitly stated otherwise all files in this repository are licensed under the Apache 2 License. | ||
| // This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2017 Datadog, Inc. | ||
| // </copyright> | ||
|
|
||
| using System; | ||
| using Datadog.Trace.Configuration; | ||
| using Datadog.Trace.TestHelpers; | ||
| using Datadog.Trace.Util; | ||
| using FluentAssertions; | ||
| using Xunit; | ||
|
|
||
| namespace Datadog.Trace.Tests.Util | ||
| { | ||
| [EnvironmentRestorer(ConfigurationKeys.Telemetry.RootSessionId)] | ||
| public class RuntimeIdTests | ||
khanayan123 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| { | ||
| [Fact] | ||
| public void RootSessionId_UsesRuntimeIdWhenNotInherited_AndInheritsWhenSet() | ||
| { | ||
| try | ||
| { | ||
| // When no env var is set, root session ID should default to runtime ID | ||
| RuntimeId.ResetForTests(); | ||
| Environment.SetEnvironmentVariable(ConfigurationKeys.Telemetry.RootSessionId, null); | ||
|
|
||
| var rootSessionId = RuntimeId.GetRootSessionId(); | ||
| rootSessionId.Should().Be(RuntimeId.Get()); | ||
|
|
||
| // When env var is pre-set (simulating a child process), root session ID | ||
| // should return the inherited value instead of the current runtime ID | ||
| var inherited = "inherited-root-session-id"; | ||
| RuntimeId.ResetForTests(); | ||
| Environment.SetEnvironmentVariable(ConfigurationKeys.Telemetry.RootSessionId, inherited); | ||
|
|
||
| RuntimeId.GetRootSessionId().Should().Be(inherited); | ||
| RuntimeId.GetRootSessionId().Should().NotBe(RuntimeId.Get()); | ||
| } | ||
| finally | ||
| { | ||
| RuntimeId.ResetForTests(); | ||
| Environment.SetEnvironmentVariable(ConfigurationKeys.Telemetry.RootSessionId, null); | ||
| RuntimeId.GetRootSessionId(); | ||
| } | ||
| } | ||
| } | ||
| } | ||
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