-
Notifications
You must be signed in to change notification settings - Fork 155
[DBM] Add container tags hash to queries (if enabled) #8061
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
4fd01fa
d9abd75
a5f3f8d
c18fc65
4ca4fdf
fdaf436
043c8b6
beb1980
a75fa60
18f4461
7a738b7
8ace897
92e4c6e
121cae0
57d31bb
3d5ed34
2161f1f
2f0eeb2
1d2679c
3d04aad
6af872d
d81eb71
335479d
747be87
a89cb3f
d3c5d7f
900ac90
f1c304e
91dc0c7
44c94a7
1c23065
d84c674
65b181a
e1eac20
e67176d
8fed285
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 |
|---|---|---|
|
|
@@ -47,7 +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 readonly ServiceRemappingHash _serviceRemappingHash; | ||
| private IApiRequestFactory _apiRequestFactory; | ||
| private AgentConfiguration? _configuration; | ||
| private string? _configurationHash; | ||
|
|
@@ -57,11 +57,12 @@ internal sealed class DiscoveryService : IDiscoveryService | |
| public DiscoveryService( | ||
| TracerSettings.SettingsManager settings, | ||
| ContainerMetadata containerMetadata, | ||
| ServiceRemappingHash serviceRemappingHash, | ||
| TimeSpan tcpTimeout, | ||
| int initialRetryDelayMs, | ||
| int maxRetryDelayMs, | ||
| int recheckIntervalMs) | ||
| : this(CreateApiRequestFactory(settings.InitialExporterSettings, containerMetadata.ContainerId, tcpTimeout), containerMetadata, initialRetryDelayMs, maxRetryDelayMs, recheckIntervalMs) | ||
| : this(CreateApiRequestFactory(settings.InitialExporterSettings, containerMetadata.ContainerId, tcpTimeout), serviceRemappingHash, initialRetryDelayMs, maxRetryDelayMs, recheckIntervalMs) | ||
| { | ||
| // Create as a "managed" service that can update the request factory | ||
| _settingSubscription = settings.SubscribeToChanges(changes => | ||
|
|
@@ -80,13 +81,13 @@ public DiscoveryService( | |
| /// </summary> | ||
| public DiscoveryService( | ||
| IApiRequestFactory apiRequestFactory, | ||
| ContainerMetadata containerMetadata, | ||
| ServiceRemappingHash serviceRemappingHash, | ||
| int initialRetryDelayMs, | ||
| int maxRetryDelayMs, | ||
| int recheckIntervalMs) | ||
| { | ||
| _apiRequestFactory = apiRequestFactory; | ||
| _containerMetadata = containerMetadata; | ||
| _serviceRemappingHash = serviceRemappingHash; | ||
| _initialRetryDelayMs = initialRetryDelayMs; | ||
| _maxRetryDelayMs = maxRetryDelayMs; | ||
| _recheckIntervalMs = recheckIntervalMs; | ||
|
|
@@ -119,10 +120,11 @@ public DiscoveryService( | |
| /// <summary> | ||
| /// Create a <see cref="DiscoveryService"/> instance that responds to runtime changes in settings | ||
| /// </summary> | ||
| public static DiscoveryService CreateManaged(TracerSettings settings, ContainerMetadata containerMetadata) | ||
| public static DiscoveryService CreateManaged(TracerSettings settings, ContainerMetadata containerMetadata, ServiceRemappingHash serviceRemappingHash) | ||
| => new( | ||
| settings.Manager, | ||
| containerMetadata, | ||
| serviceRemappingHash, | ||
| tcpTimeout: TimeSpan.FromSeconds(15), | ||
| initialRetryDelayMs: 500, | ||
| maxRetryDelayMs: 5_000, | ||
|
|
@@ -131,10 +133,11 @@ public static DiscoveryService CreateManaged(TracerSettings settings, ContainerM | |
| /// <summary> | ||
| /// Create a <see cref="DiscoveryService"/> instance that does _not_ respond to runtime changes in settings | ||
| /// </summary> | ||
| public static DiscoveryService CreateUnmanaged(ExporterSettings exporterSettings, ContainerMetadata containerMetadata) | ||
| public static DiscoveryService CreateUnmanaged(ExporterSettings exporterSettings, ContainerMetadata containerMetadata, ServiceRemappingHash serviceRemappingHash) | ||
| => CreateUnmanaged( | ||
| exporterSettings, | ||
| containerMetadata, | ||
| serviceRemappingHash, | ||
| tcpTimeout: TimeSpan.FromSeconds(15), | ||
| initialRetryDelayMs: 500, | ||
| maxRetryDelayMs: 5_000, | ||
|
|
@@ -146,13 +149,14 @@ public static DiscoveryService CreateUnmanaged(ExporterSettings exporterSettings | |
| public static DiscoveryService CreateUnmanaged( | ||
| ExporterSettings exporterSettings, | ||
| ContainerMetadata containerMetadata, | ||
| ServiceRemappingHash serviceRemappingHash, | ||
| TimeSpan tcpTimeout, | ||
| int initialRetryDelayMs, | ||
| int maxRetryDelayMs, | ||
| int recheckIntervalMs) | ||
| => new( | ||
| CreateApiRequestFactory(exporterSettings, containerMetadata.ContainerId, tcpTimeout), | ||
| containerMetadata, | ||
| serviceRemappingHash, | ||
| initialRetryDelayMs, | ||
| maxRetryDelayMs, | ||
| recheckIntervalMs); | ||
|
|
@@ -310,7 +314,7 @@ private async Task ProcessDiscoveryResponse(IApiResponse response) | |
| var containerTagsHash = response.GetHeader(AgentHttpHeaderNames.ContainerTagsHash); | ||
| if (containerTagsHash != null) | ||
| { | ||
| _containerMetadata.ContainerTagsHash = containerTagsHash; | ||
| _serviceRemappingHash.UpdateContainerTagsHash(containerTagsHash); | ||
| } | ||
|
|
||
| // Grab the original stream | ||
|
|
@@ -429,7 +433,7 @@ private async Task ProcessDiscoveryResponse(IApiResponse response) | |
| eventPlatformProxyEndpoint: eventPlatformProxyEndpoint, | ||
| telemetryProxyEndpoint: telemetryProxyEndpoint, | ||
| tracerFlareEndpoint: tracerFlareEndpoint, | ||
| containerTagsHash: _containerMetadata.ContainerTagsHash, // either the value just received, or the one we stored before (prevents overriding with null) | ||
| containerTagsHash: _serviceRemappingHash.ContainerTagsHash, // either the value just received, or the one we stored before (prevents overriding with 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. Is that correct though? 🤔 If the agent starts sending us
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 don't really see a practical case where we'd like to reset our container tags hash... Like, either they change or they stay the same, but they cannot really disappear, it'd mean that the app exited the container while still running ^^ At the same time, I don't see a real reason why the agent would reply with null, BUT it could happen if some code path don't request the value in the headers, which could happen for good reasons (we could, for performance reasons, say that we don't need the hash on every single request if we already have one). I feel like it's safer to write it that way, especialy considering how far in the code the code "requesting" the header is |
||
| clientDropP0: clientDropP0, | ||
| spanMetaStructs: spanMetaStructs, | ||
| spanEvents: spanEvents); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -250,6 +250,7 @@ public void Initialize() | |
| getDiscoveryServiceFunc: static s => DiscoveryService.CreateUnmanaged( | ||
| s.TracerSettings.Manager.InitialExporterSettings, | ||
| ContainerMetadata.Instance, | ||
| new ServiceRemappingHash(null), | ||
|
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. this one I'm not 100% sure, but since it's only used for DBM for now, I don't think it'd play any role in that code path, so it should be safe to hardcode a disabled instance
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 agree, this seems fine to me, I can't imagine we support DBM or DSM with CI Visibility today 🤔 |
||
| tcpTimeout: TimeSpan.FromSeconds(5), | ||
| initialRetryDelayMs: 100, | ||
| maxRetryDelayMs: 1000, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -533,6 +533,15 @@ supportedConfigurations: | |
| Configuration key for setting DBM propagation mode | ||
| Default value is disabled, expected values are either: disabled, service or full | ||
| <seealso cref="Datadog.Trace.Configuration.TracerSettings.DbmPropagationMode"/> | ||
| DD_DBM_INJECT_SQL_BASEHASH: | ||
|
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. Sigh this configuration key should end with |
||
| - implementation: A | ||
| type: boolean | ||
| default: 'false' | ||
| const_name: DbmInjectSqlBasehash | ||
| documentation: |- | ||
| Configuration key for enabling or disabling the injection of Base Hash in SQL Comments. | ||
| Default value is false (disabled). | ||
| <seealso cref="Datadog.Trace.Configuration.TracerSettings.DbmInjectSqlBasehash"/> | ||
| DD_DIAGNOSTIC_SOURCE_ENABLED: | ||
| - implementation: A | ||
| type: boolean | ||
|
|
||
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'm a little confused about how this interacts with the
ContainerMetadata? 🤔As I understand it
ContainerMetadataand "ContainerTagsHash" are essentially two completely different concepts at this point?ContainerMetadatais stuff we calculate, the container tags hash is something entirely agent-provided, right?(This confusion is all on me, mostly due to you having to redo things repeatedly based on my gripes 🙈)
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.
yes, correct. Container Metadata is something we can know from the tracer, and it's computed once and constant.
There are some container tags that we cannot know from inside the app, so the agent collects them and enriches the spans with them. What we do here is that we ask the agent for a key (hash) corresponding to our container tags (by providing our container ID), so that we can "reference" those container tags in places the agent cannot reach, like DSM checkpoints or DBM query comments.