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-21 20:41:26 Comparing candidate commit b920296 in PR branch Found 0 performance improvements and 2 performance regressions! Performance is the same for 25 metrics, 0 unstable metrics, 87 known flaky benchmarks.
|
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) : 70, 77
section Bailout
This PR (8164) - mean (77ms) : 75, 78
master - mean (77ms) : 75, 79
section CallTarget+Inlining+NGEN
This PR (8164) - mean (1,086ms) : 1039, 1134
master - mean (1,075ms) : 1030, 1121
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 (113ms) : 108, 118
master - mean (116ms) : 108, 123
section Bailout
This PR (8164) - mean (117ms) : 109, 125
master - mean (115ms) : 111, 118
section CallTarget+Inlining+NGEN
This PR (8164) - mean (800ms) : 770, 829
master - mean (797ms) : 773, 821
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8164) - mean (102ms) : 96, 107
master - mean (102ms) : 95, 108
section Bailout
This PR (8164) - mean (101ms) : 99, 104
master - mean (103ms) : 98, 108
section CallTarget+Inlining+NGEN
This PR (8164) - mean (945ms) : 910, 980
master - mean (940ms) : 906, 974
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8164) - mean (104ms) : 99, 109
master - mean (100ms) : 96, 103
section Bailout
This PR (8164) - mean (104ms) : 98, 109
master - mean (103ms) : 97, 108
section CallTarget+Inlining+NGEN
This PR (8164) - mean (831ms) : 775, 888
master - mean (825ms) : 786, 865
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 (192ms) : 189, 195
master - mean (191ms) : 188, 195
section Bailout
This PR (8164) - mean (196ms) : 194, 199
master - mean (195ms) : 193, 196
section CallTarget+Inlining+NGEN
This PR (8164) - mean (1,164ms) : 1114, 1214
master - mean (1,153ms) : 1101, 1205
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 (281ms) : 272, 289
master - mean (274ms) : 270, 278
section Bailout
This PR (8164) - mean (278ms) : 274, 283
master - mean (275ms) : 271, 279
section CallTarget+Inlining+NGEN
This PR (8164) - mean (948ms) : 926, 970
master - mean (930ms) : 903, 957
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8164) - mean (277ms) : 265, 289
master - mean (267ms) : 263, 272
section Bailout
This PR (8164) - mean (271ms) : 269, 274
master - mean (268ms) : 265, 271
section CallTarget+Inlining+NGEN
This PR (8164) - mean (1,150ms) : 1116, 1185
master - mean (1,138ms) : 1099, 1176
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8164) - mean (274ms) : 269, 279
master - mean (266ms) : 261, 271
section Bailout
This PR (8164) - mean (274ms) : 270, 278
master - mean (266ms) : 263, 268
section CallTarget+Inlining+NGEN
This PR (8164) - mean (1,033ms) : 989, 1076
master - mean (1,019ms) : 981, 1057
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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
Lower MinimumVersion from 1.0.0 to 0.0.0 for the CalculateTracePipeName and CalculateDogStatsDPipeName integrations so the dev (0.0.0) compat assembly is also instrumented. Add missing trimming XML entries for types newly referenced by the tracer. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…on expressions - Remove duplicated GenerateUniquePipeName from ExporterSettings, delegate to ServerlessCompatPipeNameHelper - Remove #if !NETFRAMEWORK guard from helper (no .NET Core-only APIs used) - Remove redundant Log.Information from helper (callers already log) - Use [] instead of new string[0] for ParameterTypeNames per project convention Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ions - ServerlessCompatPipeNameHelper: format, truncation, uniqueness - Integration OnMethodEnd: exception passthrough, caching, override behavior - ExporterSettings: constructor pipe name selection, generated name format Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Check for the compat binary and DLL on disk before generating pipe names in ExporterSettings static init. This prevents the tracer from switching to pipe transport when no compat layer is deployed to listen on the pipe. The version check allows 0.0.0 (dev builds) or >= 1.4.0 (first release with named pipe support). Also fix integration fallbacks to pass through the compat layer's return value instead of generating a new random GUID when no pre-generated pipe name exists. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Wrap RuntimeInformation.IsOSPlatform in #if !NETFRAMEWORK (net461 doesn't have it) - Add null-coalescing for AppDomain.CurrentDomain.BaseDirectory (nullable warning) - Remove unused argument from Log.Debug call (DDLOG003 analyzer error) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…exists When ExporterSettings hasn't pre-generated a pipe name, the integrations now generate one via ServerlessCompatPipeNameHelper instead of falling back to the compat layer's value. Also use realistic GUID-format pipe names in test fixtures to match actual compat layer behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- 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>
ceb49ad to
db18e5a
Compare
…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>
andrewlock
left a comment
There was a problem hiding this comment.
Thanks! Mostly just a few nits but there's one scenario that I think is a potential bug, where the customer configures in code, we would re-generate a different pipe name, which might mean we inject the wrong value in the compat layer
| // Use the tracer's configured pipe name, which in Azure Functions will be the | ||
| // unique name generated by ExporterSettings. Fall back to the compat layer's | ||
| // own calculated name if the tracer isn't initialized yet. | ||
| var pipeName = Tracer.Instance.Settings?.Manager?.InitialExporterSettings?.MetricsPipeName ?? returnValue; |
There was a problem hiding this comment.
nit: none of these intermediate values can be null:
| var pipeName = Tracer.Instance.Settings?.Manager?.InitialExporterSettings?.MetricsPipeName ?? returnValue; | |
| var pipeName = Tracer.Instance.Settings.Manager.InitialExporterSettings.MetricsPipeName ?? returnValue; |
| return new CallTargetReturn<string>(returnValue); | ||
| } | ||
|
|
||
| try |
There was a problem hiding this comment.
nit, tbh, It might be better to remove can probably remove the try-catch here. We catch exceptions escaping from here (and disable the integration) and we report it in telemetry, so that signal might be preferable. try-catch also means less chance of inlining, but this is a once-called thing, so not a big deal
| Log.Debug( | ||
| "ServerlessCompat integration: Overriding compat layer DogStatsD pipe name. " + | ||
| "Compat layer calculated: {CompatPipeName}, Tracer using: {TracerPipeName}", | ||
| returnValue, | ||
| pipeName); |
There was a problem hiding this comment.
Technically you're not overriding the pipe name if MetricsPipeName is null here 😄
| // Use the tracer's configured pipe name, which in Azure Functions will be the | ||
| // unique name generated by ExporterSettings. Fall back to the compat layer's | ||
| // own calculated name if the tracer isn't initialized yet. | ||
| var pipeName = Tracer.Instance.Settings?.Manager?.InitialExporterSettings?.TracesPipeName ?? returnValue; |
There was a problem hiding this comment.
Same nits as the other instrumentation 🙂
| string? tracesPipeName; | ||
| string? metricsPipeName; | ||
|
|
||
| if (AzureInfo.Instance.IsAzureFunction |
There was a problem hiding this comment.
I wish we didn't access global state here 🙁 I'm a big proponent of injecting dependencies in the constructor if we need them.
|
|
||
| if (version is null) | ||
| { | ||
| Log.Warning("Could not read Serverless Compatibility Layer details at {Path}, using fallback agent communication methods. (No Named Pipes)", compatDllPath); |
There was a problem hiding this comment.
I don't know if this should be a warning? 🤔
| Log.Warning("Could not read Serverless Compatibility Layer details at {Path}, using fallback agent communication methods. (No Named Pipes)", compatDllPath); | |
| Log.Debug("Could not read Serverless Compatibility Layer details at {Path}, using fallback agent communication methods. (No Named Pipes)", compatDllPath); |
| string? metricsPipeName; | ||
|
|
||
| if (AzureInfo.Instance.IsAzureFunction | ||
| && !AzureInfo.Instance.IsUsingSiteExtension |
There was a problem hiding this comment.
We only want this code on Windows, right? We don't want named pipes on linux etc? If so, we should add that check first, as it's cheap
| } | ||
| catch (Exception ex) | ||
| { | ||
| Log.Warning(ex, "Failed to determine Serverless Compatibility layer availability or Named Pipe Support."); |
There was a problem hiding this comment.
This one probably should be an error, no? so that we get telemetry?
| Log.Warning(ex, "Failed to determine Serverless Compatibility layer availability or Named Pipe Support."); | |
| Log.Error(ex, "Failed to determine Serverless Compatibility layer availability or Named Pipe Support."); |
| [InlineData("dogstatsd", "dd_dogstatsd_from_compat_layer")] | ||
| public void OnMethodEnd_WhenTracerHasNoPipeName_FallsBackToReturnValue(string pipeType, string compatLayerName) | ||
| { | ||
| // In a unit test environment, Tracer.Instance won't have pipe names configured, |
There was a problem hiding this comment.
In general, it's Strongly recommended you don't run code that relies on Tracer.Instance in unit tests. If you want to test this behaviour (which I'd argue would be better tested in an integration test instead), then I'd suggest extracting the code to a separate method, and testing that, and avoiding the Tracer.Instance call (which you really can't control here: insert standard thoughts on accessing global state 😄 )
| // When no explicit pipe name is configured and not in Azure Functions, | ||
| // pipe transport is not used so MetricsPipeName should be null. | ||
| // In Azure Functions, the constructor generates a unique name which | ||
| // is covered by integration tests. |
There was a problem hiding this comment.
I'm not sure it is covered by integration tests 😄
I don't actually see any "real" integration tests, that does end-to-end with named pipes - do they live in a separate repo? Is there anything more we can add there?
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