-
Notifications
You must be signed in to change notification settings - Fork 157
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 9 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 |
|---|---|---|
|
|
@@ -13,6 +13,7 @@ | |
| using Datadog.Trace.Configuration; | ||
| using Datadog.Trace.HttpOverStreams; | ||
| using Datadog.Trace.Logging; | ||
| using Datadog.Trace.PlatformHelpers; | ||
| using Datadog.Trace.Vendors.Newtonsoft.Json.Linq; | ||
|
|
||
| namespace Datadog.Trace.Agent.DiscoveryService | ||
|
|
@@ -43,23 +44,25 @@ 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; | ||
|
|
||
| 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); | ||
| } | ||
| }); | ||
|
|
@@ -71,11 +74,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; | ||
|
|
@@ -105,9 +110,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, | ||
|
|
@@ -116,9 +122,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, | ||
|
|
@@ -129,12 +136,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); | ||
|
|
@@ -254,6 +263,13 @@ int GetNextSleepDuration(int? previousDuration) => | |
|
|
||
| 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; | ||
| } | ||
|
|
||
| var jObject = await response.ReadAsTypeAsync<JObject>().ConfigureAwait(false); | ||
| if (jObject is null) | ||
| { | ||
|
|
@@ -373,13 +389,33 @@ 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 | ||
| var headers = new KeyValuePair<string, string>[AgentHttpHeaderNames.MinimalHeaders.Length + 1]; | ||
| Array.Copy(AgentHttpHeaderNames.MinimalHeaders, headers, AgentHttpHeaderNames.MinimalHeaders.Length); | ||
| headers[AgentHttpHeaderNames.MinimalHeaders.Length] = new KeyValuePair<string, string>(AgentHttpHeaderNames.ContainerId, containerId); | ||
| return headers; | ||
vandonr marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| return AgentHttpHeaderNames.MinimalHeaders; | ||
| } | ||
|
|
||
| private static IApiRequestFactory CreateApiRequestFactory(ExporterSettings exporterSettings, string? containerId, TimeSpan tcpTimeout) | ||
| { | ||
| return AgentTransportStrategy.Get( | ||
| exporterSettings, | ||
| productName: "discovery", | ||
| tcpTimeout: tcpTimeout, | ||
| AgentHttpHeaderNames.MinimalHeaders, | ||
| BuildHeaders(containerId), | ||
| () => new MinimalAgentHeaderHelper(), | ||
|
||
| uri => uri); | ||
| } | ||
| } | ||
| } | ||
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 :)