-
Notifications
You must be signed in to change notification settings - Fork 155
Read container tags hash from agent #7893
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
9a80cbf
661e5db
d565d21
1ad4b02
37a3ac1
c93c3a9
66630ec
dcb297a
653a3a5
d9abd75
a5f3f8d
c18fc65
4ca4fdf
cbe2f90
c0fd225
553b1ff
7527eb7
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 |
|---|---|---|
|
|
@@ -14,6 +14,7 @@ | |
| using Datadog.Trace.Configuration; | ||
| using Datadog.Trace.HttpOverStreams; | ||
| using Datadog.Trace.Logging; | ||
| using Datadog.Trace.PlatformHelpers; | ||
| using Datadog.Trace.SourceGenerators; | ||
| using Datadog.Trace.Util; | ||
| using Datadog.Trace.Vendors.Newtonsoft.Json.Linq; | ||
|
|
@@ -46,6 +47,7 @@ internal sealed class DiscoveryService : IDiscoveryService | |
| private readonly object _lock = new(); | ||
| private readonly Task _discoveryTask; | ||
| private readonly IDisposable? _settingSubscription; | ||
| private readonly ContainerMetadata _containerMetadata; | ||
| private IApiRequestFactory _apiRequestFactory; | ||
| private AgentConfiguration? _configuration; | ||
| private string? _configurationHash; | ||
|
|
@@ -54,18 +56,19 @@ internal sealed class DiscoveryService : IDiscoveryService | |
|
|
||
| public DiscoveryService( | ||
| TracerSettings.SettingsManager settings, | ||
| ContainerMetadata containerMetadata, | ||
| TimeSpan tcpTimeout, | ||
| int initialRetryDelayMs, | ||
| int maxRetryDelayMs, | ||
| int recheckIntervalMs) | ||
| : this(CreateApiRequestFactory(settings.InitialExporterSettings, tcpTimeout), initialRetryDelayMs, maxRetryDelayMs, recheckIntervalMs) | ||
| : this(CreateApiRequestFactory(settings.InitialExporterSettings, containerMetadata.ContainerId, tcpTimeout), containerMetadata, initialRetryDelayMs, maxRetryDelayMs, recheckIntervalMs) | ||
| { | ||
| // Create as a "managed" service that can update the request factory | ||
| _settingSubscription = settings.SubscribeToChanges(changes => | ||
| { | ||
| if (changes.UpdatedExporter is { } exporter) | ||
| { | ||
| var newFactory = CreateApiRequestFactory(exporter, tcpTimeout); | ||
| var newFactory = CreateApiRequestFactory(exporter, containerMetadata.ContainerId, tcpTimeout); | ||
| Interlocked.Exchange(ref _apiRequestFactory!, newFactory); | ||
| } | ||
| }); | ||
|
|
@@ -77,11 +80,13 @@ public DiscoveryService( | |
| /// </summary> | ||
| public DiscoveryService( | ||
| IApiRequestFactory apiRequestFactory, | ||
| ContainerMetadata containerMetadata, | ||
| int initialRetryDelayMs, | ||
| int maxRetryDelayMs, | ||
| int recheckIntervalMs) | ||
| { | ||
| _apiRequestFactory = apiRequestFactory; | ||
| _containerMetadata = containerMetadata; | ||
| _initialRetryDelayMs = initialRetryDelayMs; | ||
| _maxRetryDelayMs = maxRetryDelayMs; | ||
| _recheckIntervalMs = recheckIntervalMs; | ||
|
|
@@ -114,9 +119,10 @@ public DiscoveryService( | |
| /// <summary> | ||
| /// Create a <see cref="DiscoveryService"/> instance that responds to runtime changes in settings | ||
| /// </summary> | ||
| public static DiscoveryService CreateManaged(TracerSettings settings) | ||
| public static DiscoveryService CreateManaged(TracerSettings settings, ContainerMetadata containerMetadata) | ||
| => new( | ||
| settings.Manager, | ||
| containerMetadata, | ||
| tcpTimeout: TimeSpan.FromSeconds(15), | ||
| initialRetryDelayMs: 500, | ||
| maxRetryDelayMs: 5_000, | ||
|
|
@@ -125,9 +131,10 @@ public static DiscoveryService CreateManaged(TracerSettings settings) | |
| /// <summary> | ||
| /// Create a <see cref="DiscoveryService"/> instance that does _not_ respond to runtime changes in settings | ||
| /// </summary> | ||
| public static DiscoveryService CreateUnmanaged(ExporterSettings exporterSettings) | ||
| public static DiscoveryService CreateUnmanaged(ExporterSettings exporterSettings, ContainerMetadata containerMetadata) | ||
| => CreateUnmanaged( | ||
| exporterSettings, | ||
| containerMetadata, | ||
| tcpTimeout: TimeSpan.FromSeconds(15), | ||
| initialRetryDelayMs: 500, | ||
| maxRetryDelayMs: 5_000, | ||
|
|
@@ -138,12 +145,14 @@ public static DiscoveryService CreateUnmanaged(ExporterSettings exporterSettings | |
| /// </summary> | ||
| public static DiscoveryService CreateUnmanaged( | ||
| ExporterSettings exporterSettings, | ||
| ContainerMetadata containerMetadata, | ||
| TimeSpan tcpTimeout, | ||
| int initialRetryDelayMs, | ||
| int maxRetryDelayMs, | ||
| int recheckIntervalMs) | ||
| => new( | ||
| CreateApiRequestFactory(exporterSettings, tcpTimeout), | ||
| CreateApiRequestFactory(exporterSettings, containerMetadata.ContainerId, tcpTimeout), | ||
| containerMetadata, | ||
| initialRetryDelayMs, | ||
| maxRetryDelayMs, | ||
| recheckIntervalMs); | ||
|
|
@@ -297,6 +306,13 @@ internal bool RequireRefresh(string? currentHash, DateTimeOffset utcNow) | |
|
|
||
| private async Task ProcessDiscoveryResponse(IApiResponse response) | ||
| { | ||
| // Extract and store container tags hash from response headers | ||
| var containerTagsHash = response.GetHeader(AgentHttpHeaderNames.ContainerTagsHash); | ||
| if (containerTagsHash != null) | ||
| { | ||
| _containerMetadata.ContainerTagsHash = containerTagsHash; | ||
| } | ||
|
|
||
| // Grab the original stream | ||
| var stream = await response.GetStreamAsync().ConfigureAwait(false); | ||
|
|
||
|
|
@@ -440,13 +456,34 @@ public Task DisposeAsync() | |
| return _discoveryTask; | ||
| } | ||
|
|
||
| private static IApiRequestFactory CreateApiRequestFactory(ExporterSettings exporterSettings, TimeSpan tcpTimeout) | ||
| => AgentTransportStrategy.Get( | ||
| /// <summary> | ||
| /// Builds the headers array for the discovery service, including the container ID if available. | ||
| /// Internal for testing purposes. | ||
| /// </summary> | ||
| internal static KeyValuePair<string, string>[] BuildHeaders(string? containerId) | ||
| { | ||
| if (containerId != null) | ||
|
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. We never "discover" this later, right? i.e. if
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. yes correct. Are you asking because if yes, then you'd have this code written differently, or just out of curiosity ?
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. Yeah, if the answer had been "yes" we would have to write this differently, because we're building this collection once and never re-evaluating. Which is fine if this always gives the same result, but not otherwise 🙂 |
||
| { | ||
| // if container ID is available, add it to headers | ||
| return | ||
| [ | ||
| ..AgentHttpHeaderNames.MinimalHeaders, | ||
| new(AgentHttpHeaderNames.ContainerId, containerId), | ||
| ]; | ||
| } | ||
|
|
||
| return AgentHttpHeaderNames.MinimalHeaders; | ||
| } | ||
|
|
||
| private static IApiRequestFactory CreateApiRequestFactory(ExporterSettings exporterSettings, string? containerId, TimeSpan tcpTimeout) | ||
| { | ||
| return AgentTransportStrategy.Get( | ||
| exporterSettings, | ||
| productName: "discovery", | ||
| tcpTimeout: tcpTimeout, | ||
| AgentHttpHeaderNames.MinimalHeaders, | ||
| () => new MinimalAgentHeaderHelper(), | ||
| BuildHeaders(containerId), | ||
| () => new MinimalAgentHeaderHelper(containerId), | ||
| uri => uri); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,13 @@ namespace Datadog.Trace.HttpOverStreams; | |
| internal class MinimalAgentHeaderHelper : HttpHeaderHelperBase | ||
| { | ||
| private static string? _metadataHeaders = null; | ||
| private static string? _metadataHeadersWithContainerId = null; | ||
| private readonly string? _containerId; | ||
|
|
||
| public MinimalAgentHeaderHelper(string? containerId = null) | ||
| { | ||
| _containerId = containerId; | ||
| } | ||
|
|
||
| protected override string MetadataHeaders | ||
| { | ||
|
|
@@ -22,6 +29,18 @@ protected override string MetadataHeaders | |
| _metadataHeaders = string.Concat(headers); | ||
| } | ||
|
|
||
| if (_containerId != null) | ||
|
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 we should probably refactor the
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. ok thanks. Yeah that code felt a bit... dated |
||
| { | ||
| if (_metadataHeadersWithContainerId == null) | ||
| { | ||
| // Assuming we'd always get the same container ID. The first time, we use its value to build the header, | ||
| // then it's just a marker that we want the header with the containerID in it, and we don't look at its value. | ||
| _metadataHeadersWithContainerId = _metadataHeaders + $"{AgentHttpHeaderNames.ContainerId}: {_containerId}{DatadogHttpValues.CrLf}"; | ||
| } | ||
|
|
||
| return _metadataHeadersWithContainerId; | ||
| } | ||
|
|
||
| return _metadataHeaders; | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |
| // This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2017 Datadog, Inc. | ||
| // </copyright> | ||
|
|
||
| #nullable enable | ||
|
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. 🙏 |
||
| #if !NETFRAMEWORK | ||
|
|
||
| using System; | ||
|
|
@@ -43,28 +44,39 @@ internal sealed class ContainerMetadata | |
|
|
||
| public static readonly ContainerMetadata Instance = new(); | ||
|
|
||
| private readonly Lazy<string> _containerId; | ||
| private readonly Lazy<string> _entityId; | ||
| private readonly Lazy<string?> _containerId; | ||
| private readonly Lazy<string?> _entityId; | ||
|
|
||
| private ContainerMetadata() | ||
| { | ||
| _containerId = new Lazy<string>(GetContainerIdInternal, LazyThreadSafetyMode.ExecutionAndPublication); | ||
| _entityId = new Lazy<string>(() => GetEntityIdInternal(_containerId), LazyThreadSafetyMode.ExecutionAndPublication); | ||
| _containerId = new Lazy<string?>(GetContainerIdInternal, LazyThreadSafetyMode.ExecutionAndPublication); | ||
| _entityId = new Lazy<string?>(() => GetEntityIdInternal(_containerId), LazyThreadSafetyMode.ExecutionAndPublication); | ||
| } | ||
|
|
||
| // For use in tests only | ||
| public ContainerMetadata(string containerId, string entityId) | ||
| [TestingOnly] | ||
| public ContainerMetadata(string? containerId, string? entityId) | ||
vandonr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| { | ||
| _containerId = new Lazy<string>(() => containerId); | ||
| _entityId = new Lazy<string>(() => entityId); | ||
| _containerId = new Lazy<string?>(() => containerId); | ||
| _entityId = new Lazy<string?>(() => entityId); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets the container tags hash received from the agent, used by DBM/DSM | ||
| /// This is set when we receive a value for it in an http response from the agent | ||
| /// </summary> | ||
| public string? ContainerTagsHash | ||
| { | ||
| get => Volatile.Read(ref field); | ||
| set => Volatile.Write(ref field, value); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets the id of the container executing the code. | ||
| /// Return <c>null</c> if code is not executing inside a supported container. | ||
| /// </summary> | ||
| /// <value>The container id or <c>null</c>.</value> | ||
| public string ContainerId | ||
| public string? ContainerId | ||
| { | ||
| get => _containerId.Value; | ||
| } | ||
|
|
@@ -80,7 +92,7 @@ public string ContainerId | |
| /// </list> | ||
| /// </summary> | ||
| /// <value>The entity id or <c>null</c>.</value> | ||
| public string EntityId | ||
| public string? EntityId | ||
| { | ||
| get => _entityId.Value; | ||
| } | ||
|
|
@@ -90,7 +102,7 @@ public string EntityId | |
| /// </summary> | ||
| /// <param name="lines">Lines of text from a cgroup file.</param> | ||
| /// <returns>The container id if found; otherwise, <c>null</c>.</returns> | ||
| public static string ParseContainerIdFromCgroupLines(IEnumerable<string> lines) | ||
| public static string? ParseContainerIdFromCgroupLines(IEnumerable<string> lines) | ||
| { | ||
| return lines.Select(ParseContainerIdFromCgroupLine) | ||
| .FirstOrDefault(id => !string.IsNullOrWhiteSpace(id)); | ||
|
|
@@ -101,7 +113,7 @@ public static string ParseContainerIdFromCgroupLines(IEnumerable<string> lines) | |
| /// </summary> | ||
| /// <param name="line">A single line from a cgroup file.</param> | ||
| /// <returns>The container id if found; otherwise, <c>null</c>.</returns> | ||
| public static string ParseContainerIdFromCgroupLine(string line) | ||
| public static string? ParseContainerIdFromCgroupLine(string line) | ||
| { | ||
| var lineMatch = Regex.Match(line, ContainerIdRegex); | ||
|
|
||
|
|
@@ -119,7 +131,7 @@ public static string ParseContainerIdFromCgroupLine(string line) | |
| /// <param name="controlGroupsMountPath">Path to the cgroup mount point.</param> | ||
| /// <param name="lines">Lines of text from a cgroup file.</param> | ||
| /// <returns>The cgroup node controller's inode if found; otherwise, <c>null</c>.</returns> | ||
| public static string ExtractInodeFromCgroupLines(string controlGroupsMountPath, IEnumerable<string> lines) | ||
| public static string? ExtractInodeFromCgroupLines(string controlGroupsMountPath, IEnumerable<string> lines) | ||
| { | ||
| foreach (var line in lines) | ||
| { | ||
|
|
@@ -147,7 +159,7 @@ public static string ExtractInodeFromCgroupLines(string controlGroupsMountPath, | |
| /// </summary> | ||
| /// <param name="line">A single line from a cgroup file.</param> | ||
| /// <returns>The controller/cgroup-node-path pair if found; otherwise, <c>null</c>.</returns> | ||
| public static Tuple<string, string> ParseControllerAndPathFromCgroupLine(string line) | ||
| public static Tuple<string, string>? ParseControllerAndPathFromCgroupLine(string line) | ||
| { | ||
| var lineMatch = Regex.Match(line, CgroupRegex); | ||
|
|
||
|
|
@@ -181,7 +193,7 @@ internal static bool TryGetInodeUsingPInvoke(string path, out long result) | |
| return false; | ||
| } | ||
|
|
||
| static void LogError(Exception ex, string message) | ||
| static void LogError(Exception? ex, string message) | ||
| { | ||
| #pragma warning disable DDLOG004 // Must use constant strings - disabled as it's an integer only, and only called twice in the app lifetime | ||
| if (EnvironmentHelpersNoLogging.IsClrProfilerAttachedSafe()) | ||
|
|
@@ -215,7 +227,7 @@ internal static bool TryGetInodeUsingStat(string path, out long result) | |
| } | ||
| } | ||
|
|
||
| private static string GetContainerIdInternal() | ||
| private static string? GetContainerIdInternal() | ||
| { | ||
| try | ||
| { | ||
|
|
@@ -236,7 +248,7 @@ private static string GetContainerIdInternal() | |
| return null; | ||
| } | ||
|
|
||
| private static string GetEntityIdInternal(Lazy<string> lazyContainerId) | ||
| private static string? GetEntityIdInternal(Lazy<string?> lazyContainerId) | ||
| { | ||
| if (lazyContainerId.Value is string containerId) | ||
| { | ||
|
|
@@ -252,7 +264,7 @@ private static string GetEntityIdInternal(Lazy<string> lazyContainerId) | |
| } | ||
| } | ||
|
|
||
| private static string GetCgroupInode() | ||
| private static string? GetCgroupInode() | ||
| { | ||
| try | ||
| { | ||
|
|
||
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.
I was wondering about this code, as we're going to do the work to extract and set this hash with every response, but I think that's fine, given that we shouldn't call the discovery service so much any more since #7979 (and also given the amount of work we do later in this method)
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.
yeah, since it's not supposed to change, we could check if we already have it, and only query it then. But if you say it's fine :)