Add Multi-Azure Function Unique Named Pipe Configuration#8164
Add Multi-Azure Function Unique Named Pipe Configuration#8164
Conversation
37f021e to
1b6956c
Compare
BenchmarksBenchmark execution time: 2026-04-23 22:28:07 Comparing candidate commit efe17b2 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 27 metrics, 0 unstable metrics, 60 known flaky benchmarks, 27 flaky benchmarks without significant changes.
|
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8164) and master. ✅ No regressions detected - check the details below Full Metrics ComparisonFakeDbCommand
HttpMessageHandler
Comparison explanationExecution-time benchmarks measure the whole time it takes to execute a program, and are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are highlighted in **red**. The following thresholds were used for comparing the execution times:
Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard. Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph). Duration chartsFakeDbCommand (.NET Framework 4.8)gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8164) - mean (73ms) : 70, 75
master - mean (73ms) : 69, 77
section Bailout
This PR (8164) - mean (79ms) : 75, 83
master - mean (80ms) : 76, 84
section CallTarget+Inlining+NGEN
This PR (8164) - mean (1,081ms) : 1030, 1132
master - mean (1,081ms) : 1038, 1124
FakeDbCommand (.NET Core 3.1)gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8164) - mean (114ms) : 110, 119
master - mean (117ms) : 111, 123
section Bailout
This PR (8164) - mean (120ms) : 113, 126
master - mean (118ms) : 112, 124
section CallTarget+Inlining+NGEN
This PR (8164) - mean (778ms) : 742, 814
master - mean (779ms) : 749, 810
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8164) - mean (106ms) : 98, 113
master - mean (102ms) : 98, 106
section Bailout
This PR (8164) - mean (103ms) : 100, 107
master - mean (106ms) : 99, 112
section CallTarget+Inlining+NGEN
This PR (8164) - mean (944ms) : 901, 986
master - mean (942ms) : 908, 976
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8164) - mean (101ms) : 95, 107
master - mean (101ms) : 97, 104
section Bailout
This PR (8164) - mean (104ms) : 98, 110
master - mean (101ms) : 99, 104
section CallTarget+Inlining+NGEN
This PR (8164) - mean (824ms) : 780, 868
master - mean (823ms) : 788, 858
HttpMessageHandler (.NET Framework 4.8)gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8164) - mean (203ms) : 195, 211
master - mean (206ms) : 196, 216
section Bailout
This PR (8164) - mean (207ms) : 201, 214
master - mean (210ms) : 201, 219
section CallTarget+Inlining+NGEN
This PR (8164) - mean (1,196ms) : 1140, 1252
master - mean (1,211ms) : 1144, 1277
HttpMessageHandler (.NET Core 3.1)gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8164) - mean (290ms) : 279, 300
master - mean (294ms) : 281, 308
section Bailout
This PR (8164) - mean (291ms) : 280, 301
master - mean (296ms) : 281, 310
section CallTarget+Inlining+NGEN
This PR (8164) - mean (957ms) : 920, 994
master - mean (962ms) : 927, 997
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8164) - mean (284ms) : 275, 293
master - mean (285ms) : 277, 293
section Bailout
This PR (8164) - mean (287ms) : 275, 298
master - mean (287ms) : 274, 300
section CallTarget+Inlining+NGEN
This PR (8164) - mean (1,153ms) : 1102, 1204
master - mean (1,155ms) : 1116, 1194
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8164) - mean (283ms) : 273, 293
master - mean (285ms) : 275, 294
section Bailout
This PR (8164) - mean (283ms) : 274, 292
master - mean (285ms) : 276, 293
section CallTarget+Inlining+NGEN
This PR (8164) - mean (1,044ms) : 997, 1091
master - mean (1,048ms) : 974, 1121
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
aaff9b5 to
08a6c72
Compare
2fcae54 to
edb4121
Compare
duncanpharvey
left a comment
There was a problem hiding this comment.
We check the presence of the compat layer + windows configs (using undocumented envvar DD_SERVERLESS_COMPAT_BINARY_PATH if necessary to adapt to Azure changes) to see if named pipes can be used or if we should rely on TCP
What is meant by this? I don't see DD_SERVERLESS_COMPAT_BINARY_PATH in any of the PR changes.
|
Should this dependency for |
|
@codex review |
| && !Util.EnvironmentHelpers.IsUsingAzureAppServicesSiteExtension() | ||
| && IsCompatLayerAvailableWithPipeSupport()) | ||
| { | ||
| var baseName = Util.EnvironmentHelpers.GetEnvironmentVariable(ConfigurationKeys.TracesPipeName); |
There was a problem hiding this comment.
This is only reading the "base" name from environment variables, but the tracer can be configured in other ways (e.g. config files), and those configs would be ignored there. Instead of reading from env vars directly, I think you can read the user setting from rawSettings.TracesPipeName, passed in via parameter.
There was a problem hiding this comment.
Agreed, but we also need to make sure we record the new "calculated" value in telemetry as well
There was a problem hiding this comment.
Looks like my comment is addressed (no reading env vars directly), but I'll leave this open for Andrew's comment (configuration telemetry).
andrewlock
left a comment
There was a problem hiding this comment.
LGTM in general, but there's some specific things to address before we merge it (mostly already highlighted by Lucas)
- We should only target existing major versions, and bump support later, otherwise we need strict backward and forward compatibility, which is very hard to maintain and very easy to screw up 😅
- We shouldn't have static state if we can help it. If we want to cache things, we typically handle that by:
- Move the calculating and storage of the value to a helper type as instance methods.
- Create a "singleton" instance of the helper
- Pass the singleton in to the constructor (or, if this gives too much blast radius, allow passing in a null value for the helper, and grab the singleton in the body. This isn't the preferred approach, but it's sometimes the practical one)
- We need to ensure we record the new calculated values in telemetry
- The instrumentation seems a little overly complex considering the behaviours we need and the failure cases
| && !Util.EnvironmentHelpers.IsUsingAzureAppServicesSiteExtension() | ||
| && IsCompatLayerAvailableWithPipeSupport()) | ||
| { | ||
| var baseName = Util.EnvironmentHelpers.GetEnvironmentVariable(ConfigurationKeys.TracesPipeName); |
There was a problem hiding this comment.
Agreed, but we also need to make sure we record the new "calculated" value in telemetry as well
c9b7aee to
1df4f5a
Compare
- Fix "Compatability" → "Compatibility" typo (3 occurrences) - Use `is null` / `is not null` instead of `== null` / `!= null` - Use `StringUtil.IsNullOrEmpty()` instead of `string.IsNullOrEmpty()` - Remove duplicate test file in Datadog.Trace.Tests - Remove tests already covered by ServerlessCompatPipeNameHelperTests - Fix SA1508 blank line before closing brace in PlatformKeys Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…nique pipe generation Instead of skipping pipe generation when an explicit pipe name is configured, use the configured value as the base name with a GUID suffix appended. This allows users to customize the pipe base name while still getting per-instance isolation on shared Azure Functions hosting plans. If no explicit pipe name is set, falls back to "dd_trace" / "dd_dogstatsd". Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… integrations CalculateDogStatsDPipeName and CalculateTracePipeName are each only called once during Start(), so caching is unnecessary. The fallback to generate a new unique name is also removed since ExporterSettings always pre-generates the pipe name before these instrumentations fire, and generating a name the tracer doesn't know about is pointless — fall back to the compat layer's own value instead. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…e constructor Replace static field initializers that read env vars directly with instance methods that use rawSettings.TracesPipeName/MetricsPipeName, respecting all config sources (env vars, config files, etc.) instead of only environment variables. Record the calculated pipe names in telemetry. Integrations now read pipe names from Tracer.Instance.Settings.Exporter instead of removed static properties. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…e generation Check the Azure Functions/compat layer condition once in the constructor instead of duplicating it in two separate methods. Consolidate the two per-pipe methods into a single GenerateUniquePipeName overload. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…eHelper and make testable Move compat layer detection out of ExporterSettings into ServerlessCompatPipeNameHelper alongside the related pipe name logic. Extract I/O dependencies (file existence, assembly version) as delegates so version-checking logic can be fully unit tested. Add support for DD_SERVERLESS_COMPAT_PATH env var to override the default binary path, matching the compat layer's own behavior. Add comprehensive tests for all branches: version checks, file missing, null version, exceptions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… fix ExporterSettings access path - Add DD_SERVERLESS_COMPAT_PATH to supported-configurations.yaml so the source generator creates ConfigurationKeys.ServerlessCompatPath - Fix integration classes to use the correct property path: Tracer.Instance.Settings.Manager.InitialExporterSettings - Restore using Datadog.Trace.Configuration (needed for IntegrationId) - Remove unused System.Reflection/InteropServices from ExporterSettings Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
+ Maximum Version change Co-authored-by: Andrew Lock <andrewlock.net@gmail.com>
StringUtil.IsNullOrEmpty has [NotNullWhen(false)] so the compiler already knows the value is non-null in the false branch. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…SERVERLESS_COMPAT_PATH config key Update supported_calltargets.g.json and generated_calltargets.g.cpp to reflect MaximumVersion change from 2.*.* to 1.*.*. Add DD_SERVERLESS_COMPAT_PATH to supported-configurations.yaml and restore ConfigurationKeys usage in ServerlessCompatPipeNameHelper. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Version 1.4.0 adds the CalculateTracePipeName and CalculateDogStatsDPipeName methods that the tracer instruments for named pipe coordination. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ests - Update version range comments from 2.*.* to 1.*.* in both integrations - Update DD_SERVERLESS_COMPAT_PATH documentation for linux default path - Rewrite ServerlessCompatIntegrationTests: remove reflection-based tests for removed cache fields, add Theory-based tests for exception passthrough and fallback behavior with exact value assertions Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…typo, use SkipOn in tests - Move ServerlessCompatPipeNameHelper from ClrProfiler.AutoInstrumentation.Serverless to Datadog.Trace.Serverless to avoid Configuration depending on instrumentation code - Fix "compatability" -> "compatibility" typo in YAML documentation - Replace silent early-return platform guards with SkipOn/SkippableFact so Windows-only tests show as skipped instead of silently passing Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Compat 1.4.0 isn't published yet and has a dependency on these tracer changes. Keep the 1.2.0 dependency — the instrumentation silently skips when the target methods don't exist, and IsCompatLayerAvailableWithPipeSupport gates on >= 1.4.0 at runtime. The dependency will be bumped when compat 1.4.0 is published. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Master #8231 extracted serverless platform detection from EnvironmentHelpers into dedicated cached classes under Datadog.Trace.Serverless. Update the Azure-Functions pipe-name generation gate to call AzureInfo.Instance.IsAzureFunction and IsUsingSiteExtension instead of the removed EnvironmentHelpers methods. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses review feedback: a customer-set TracesPipeName/MetricsPipeName (from any configuration source) should pass through verbatim even when running in Azure Functions, even though this can cause pipe-name collisions across multiple function instances sharing a plan. We only fabricate a GUID-suffixed default pipe name when the customer has not configured one themselves. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rebase Post-rebase reconciliation: master's trace_filter additions took conflict priority during rebase, dropping the ServerlessCompat integration_name tag entries. Re-add them in both integration_name tag groups and shift downstream indices by +2. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previous attempt missed `var index = N + ((int)tag1 * M) + (int)tag2;` forms. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Regenerated via Nuke (Restore + CompileManagedSrc). Both integration_name tag groups now reflect 85 entries instead of 84. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Customer code calling Tracer.Configure triggers ExporterSettings reconstruction. With the previous static-GUID generator, each rebuild produced a new pipe name that no longer matched what the Serverless Compat layer already bound to. SettingsManager now computes the gating (Windows + Azure Function + compat layer 1.4+) once at startup, generates GUIDs, and freezes them into an immutable ServerlessCompatPipeNames record passed to every ExporterSettings construction. DD_SERVERLESS_COMPAT_PATH moved into Raw via the telemetry-enabled ConfigurationBuilder so it shows up in config telemetry. Integration callbacks drop try/catch + null chains and short-circuit when no tracer pipe name is configured. Log levels aligned with Andrew's review (Debug for missing compat layer, Error for unexpected failures). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per Andrew's review: unit tests should not depend on Tracer.Instance global state. The removed case relied on Tracer.Instance defaulting to unconfigured pipe names, which is unsafe — any prior test could have left the singleton in an unrelated state. The non-exception path is covered end-to-end by integration tests elsewhere. The exception-guard test stays since it short-circuits before reaching Tracer.Instance. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The helper's only caller (SettingsManager.CreatePipeNames) already does the Windows short-circuit as the first gate in its condition chain. The internal RuntimeInformation.IsOSPlatform check was unreachable in practice and duplicated the rule in two places. Drops the System.Runtime.InteropServices import and the #if !NETFRAMEWORK guard. Tests that assumed a platform-gated behavior are now cross-platform, bringing 10 previously-skipped cases into every run. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
4a4b1ba to
0587f49
Compare
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
| { | ||
| /// <summary> | ||
| /// Checks whether the Datadog Serverless Compat layer is available and supports named-pipe | ||
| /// transport. Pipe-name generation itself lives in <see cref="ServerlessCompatPipeNames"/>. |
There was a problem hiding this comment.
Pipe-name generation itself lives in
<see cref="ServerlessCompatPipeNames"/>
Is this comment accurate? ServerlessCompatPipeNames looks like a record type with no logic.
andrewlock
left a comment
There was a problem hiding this comment.
LGTM, thanks! A couple of remaining nit suggestions, but definitely non-blocking, just feels 😄
| // Customer-configured pipe names always take precedence; otherwise use whatever the | ||
| // SettingsManager pre-computed for the Serverless Compat layer (may be null on | ||
| // non-Windows / non-Azure-Function / missing-compat-layer runs). | ||
| var tracesPipeName = !StringUtil.IsNullOrEmpty(rawSettings.TracesPipeName) | ||
| ? rawSettings.TracesPipeName | ||
| : pipeNames.TracesPipeName; | ||
| var metricsPipeName = !StringUtil.IsNullOrEmpty(rawSettings.MetricsPipeName) | ||
| ? rawSettings.MetricsPipeName | ||
| : pipeNames.MetricsPipeName; | ||
|
|
||
| RecordCalculatedPipeName(ConfigurationKeys.TracesPipeName, tracesPipeName, rawSettings.TracesPipeName); | ||
| RecordCalculatedPipeName(ConfigurationKeys.MetricsPipeName, metricsPipeName, rawSettings.MetricsPipeName); |
There was a problem hiding this comment.
super tiny (optional) nit, I would personally probably combine the "recording" with the actual reading of the setting - extracting a small function which also embeds some logic as to what to record probably isn't a net win IMO. 🤷♂️ e.g.
| // Customer-configured pipe names always take precedence; otherwise use whatever the | |
| // SettingsManager pre-computed for the Serverless Compat layer (may be null on | |
| // non-Windows / non-Azure-Function / missing-compat-layer runs). | |
| var tracesPipeName = !StringUtil.IsNullOrEmpty(rawSettings.TracesPipeName) | |
| ? rawSettings.TracesPipeName | |
| : pipeNames.TracesPipeName; | |
| var metricsPipeName = !StringUtil.IsNullOrEmpty(rawSettings.MetricsPipeName) | |
| ? rawSettings.MetricsPipeName | |
| : pipeNames.MetricsPipeName; | |
| RecordCalculatedPipeName(ConfigurationKeys.TracesPipeName, tracesPipeName, rawSettings.TracesPipeName); | |
| RecordCalculatedPipeName(ConfigurationKeys.MetricsPipeName, metricsPipeName, rawSettings.MetricsPipeName); | |
| // Customer-configured pipe names always take precedence; otherwise use whatever the | |
| // SettingsManager pre-computed for the Serverless Compat layer (may be null on | |
| // non-Windows / non-Azure-Function / missing-compat-layer runs). | |
| string? tracesPipeName = null; | |
| string? metricsPipeName = null; | |
| if (!StringUtil.IsNullOrEmpty(rawSettings.TracesPipeName)) | |
| { | |
| tracesPipeName = rawSettings.TracesPipeName; | |
| } | |
| else if (pipeNames.TracesPipeName is { } pipeName) | |
| { | |
| tracesPipeName = pipeName; | |
| _telemetry.Record(ConfigurationKeys.TracesPipeName, pipeName, recordValue: true, ConfigurationOrigins.Calculated); | |
| } | |
| if (!StringUtil.IsNullOrEmpty(rawSettings.MetricsPipeName)) | |
| { | |
| tracesPipeName = rawSettings.MetricsPipeName; | |
| } | |
| else if (pipeNames.TracesPipeName is { } metricsName) | |
| { | |
| tracesPipeName = pipeName; | |
| _telemetry.Record(ConfigurationKeys.MetricsPipeName, metricsName, recordValue: true, ConfigurationOrigins.Calculated); | |
| } |
| // DD_SERVERLESS_COMPAT_PATH overrides the default binary location | ||
| // (matches CompatibilityLayer.cs in datadog-serverless-compat-dotnet). | ||
| const string defaultCompatBinaryPath = @"C:\home\site\wwwroot\datadog\bin\windows-amd64\datadog-serverless-compat.exe"; | ||
| var compatBinaryPath = !string.IsNullOrEmpty(compatPathOverride) ? compatPathOverride! : defaultCompatBinaryPath; |
There was a problem hiding this comment.
nit: if you use StringUtil here, you don't need the ! dammit
| var compatBinaryPath = !string.IsNullOrEmpty(compatPathOverride) ? compatPathOverride! : defaultCompatBinaryPath; | |
| var compatBinaryPath = !StringUtil.IsNullOrEmpty(compatPathOverride) ? compatPathOverride : defaultCompatBinaryPath; |
Summary of changes
Reason for change
Implementation details
For Azure Functions, the tracer loads first, then the compat layer, and then the compat layer launches a rust binary to act as the agent. Additionally, the client may or may not be using the DogstatsD client, and it is also possible to have the compat layer and DogstatsD in place without the tracer. We need to coordinate named pipes across all three/four of these codebases, noting that the tracer will use DogstatsD's pipe for runtime metrics if available.
Given this order of operations, and that we want this to work with immutable configuration, the tracer should set pipe names for the compatibility layer via instrumentation if the tracer is present.
DD_SERVERLESS_COMPAT_BINARY_PATHif necessary to adapt to Azure changes) to see if named pipes can be used or if we should rely on TCPTest coverage