fix(libdd-common): Add fallback logic for resolving Azure Functions instance name [SVLS-8931]#2077
Conversation
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: aeffe22 | Docs | Datadog PR Page | Give us feedback! |
Clippy Allow Annotation ReportComparing clippy allow annotations between branches:
Summary by Rule
Annotation Counts by File
Annotation Stats by Crate
About This ReportThis report tracks Clippy allow annotations for specific rules, showing how they've changed in this PR. Decreasing the number of these annotations generally improves code quality. |
kathiehuang
changed the title
kathiehuang
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2077 +/- ##
==========================================
+ Coverage 73.49% 73.52% +0.03%
==========================================
Files 475 475
Lines 78994 79143 +149
==========================================
+ Hits 58054 58191 +137
- Misses 20940 20952 +12
🚀 New features to boost your workflow:
|
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Pull request overview
This PR moves Azure Functions instance-name resolution into libdd-common by adding hosting-plan-aware fallback logic so aas.environment.instance_name is populated correctly across Flex Consumption/Consumption and Premium/Dedicated plans. It also exposes a few environment variable name constants for reuse by downstream components (e.g., serverless-components).
Changes:
- Add
resolve_instance_nameto select the correct instance identifier source (CONTAINER_NAME/WEBSITE_POD_NAME/COMPUTERNAME) based onWEBSITE_SKU, with fallbacks. - Expand the Linux
/proc/self/environallowlist to include the new instance-name env vars. - Make selected constants public and add/adjust unit tests to cover the new behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn non_empty(s: Option<&str>) -> Option<&str> { | ||
| s.filter(|v| !v.trim().is_empty()) | ||
| } |
kathiehuang
force-pushed
the
kathie.huang/update-azure-function-instance-name-logic
branch
from
69e67aa to
fcdcb31
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fcdcb31d3e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .or_else(|| non_empty(computer_name)) | ||
| .or_else(|| non_empty(container_name)) | ||
| .or_else(|| non_empty(pod_name)) | ||
| .map(|s| s.to_string()) |
There was a problem hiding this comment.
In resolve_instance_name, mixed-case Azure values are returned as-is; for example the new tests cover CONTAINER_NAME="0--abc-DEF" and "ABCD...", but the serverless enhanced instance metric this logic is being moved from lowercases the resolved instance tag before submission. In Flex/Consumption or Premium plans where Azure supplies uppercase characters, spans will get aas.environment.instance_name with different casing than the azure.functions.enhanced.instance tag, breaking the intended correlation even though the same source env var was selected.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is intentional - the existing aas.environment.instance_name span attribute preserves the casing, but on the integration metric, the instance tag is consistently lowercased.
Example:
Mixed case in existing instance name span attribute:
Lowercase in integration metric azure.functions.function_execution_count (pulled from Azure Monitor):
| } | ||
|
|
||
| #[test] | ||
| fn test_public_consts_are_accessible() { |
There was a problem hiding this comment.
Removed the test since it's not useful 1322cf3
kathiehuang
force-pushed
the
kathie.huang/update-azure-function-instance-name-logic
branch
from
1322cf3 to
ee2b343
Compare
…the instance name
kathiehuang
force-pushed
the
kathie.huang/update-azure-function-instance-name-logic
branch
from
ee2b343 to
ff8ee41
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff8ee413f2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Some("FlexConsumption") | Some("Dynamic") => { | ||
| non_empty(container_name).or_else(|| non_empty(pod_name)) | ||
| } |
There was a problem hiding this comment.
When a Windows Consumption function has WEBSITE_SKU=Dynamic and also exposes a CONTAINER_NAME app setting, this branch chooses CONTAINER_NAME before COMPUTERNAME. The new Windows test only covers the case where container vars are absent, but app settings are surfaced as environment variables, so a user-defined CONTAINER_NAME would overwrite aas.environment.instance_name with an unrelated value instead of the Windows instance name the PR says should come from COMPUTERNAME; the resolver needs an OS/plan signal or a Windows-specific preference.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Hmmm. Maybe I can remove WEBSITE_SKU from the function signature entirely and just make the fallback logic the same regardless of the hosting plan - COMPUTERNAME > WEBSITE_POD_NAME > CONTAINER_NAME. Based off this investigation, these three environment variables never conflict - they're either the same as each other or empty. This ordering makes the most likely env var to be set by a user, CONTAINER_NAME, the last in the fallback chain.
There was a problem hiding this comment.
Deployed with serverless-compat-self-monitoring and ensured no regressions
| pub const WEBSITE_SKU: &str = "WEBSITE_SKU"; | ||
| pub const REGION_NAME: &str = "REGION_NAME"; | ||
| const CONTAINER_NAME: &str = "CONTAINER_NAME"; | ||
| const WEBSITE_POD_NAME: &str = "WEBSITE_POD_NAME"; | ||
|
|
||
| const UNKNOWN_VALUE: &str = "unknown"; | ||
| pub const UNKNOWN_VALUE: &str = "unknown"; |
There was a problem hiding this comment.
Do these constants need to be public?
| let container_name = query.get_var(CONTAINER_NAME); | ||
| let pod_name = query.get_var(WEBSITE_POD_NAME); | ||
| let computer_name = query.get_var(INSTANCE_NAME); |
There was a problem hiding this comment.
Very minor nit but I think it would make it easier to follow the instance name resolution order if the variables were defined in the same order (computer_name -> pod_name -> container_name).
There was a problem hiding this comment.
Good point! Made the order consistent in aeffe22
There was a problem hiding this comment.
Maybe the INSTANCE_NAME constant should be renamed to COMPUTERNAME given that instance name can come from one of several environment variables now.
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: queued
mergequeue-status: in_progress
and removed
mergequeue-status: queued
labels
gh-worker-dd-mergequeue-cf854d
Bot
deleted the
kathie.huang/update-azure-function-instance-name-logic
branch
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: done
and removed
mergequeue-status: in_progress
labels
# Release proposal for libdd-remote-config and its dependencies This PR contains version bumps based on public API changes and commits since last release. ## libdd-common **Next version:** `5.0.0` **Semver bump:** `major` **Tag:** `libdd-common-v5.0.0` ### Commits - chore(profiling): Use SECURITY_ANONYMOUS when connecting to named pipe server (#2134) - fix: Fix http PathAndQuery Uri Parsing (#2122) - chore(common)!: replace native-certs with platform-verifier (#2078) - feat(data-pipeline)!: CSS Trace Filters (#1985) - fix(libdd-common): Add fallback logic for resolving Azure Functions instance name [SVLS-8931] (#2077) - test: fix timeouts on heavily contended scenarios (#2093) ## libdd-remote-config **Next version:** `1.0.0` **Semver bump:** `major` **Tag:** `libdd-remote-config-v1.0.0` **Warning:** this is an initial release. Please verify that the version and commits included are correct. --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: iunanua <18325288+iunanua@users.noreply.github.com>
What does this PR do?
COMPUTERNAME->WEBSITE_POD_NAME->CONTAINER_NAMEinstancetag ofazure.functions.function_execution_countintegration metricCOMPUTERNAME. The investigation to determine which env vars should be used is in the Instance tab of Enhanced Metrics in the Serverless Compatibility Layeraas.environment.instance_namein hosting plans where the attribute wasunknownbeforeCOMPUTERNAMEfor instance name, which was empty for Linux Flex Consumption and Consumption functionsMotivation
https://datadoghq.atlassian.net/browse/SVLS-8931
https://datadoghq.atlassian.net/browse/SVLS-9015
Additional Notes
Once this PR is merged, we plan to make a new PR in serverless-components to update the libdatadog commit hash and remove the redundant consts and instance name logic. Draft PR: DataDog/serverless-components#134
How to test the change?
Unit tests: Run
cargo test -p libdd-common azure_app_servicesFlex Consumption function with fix:
Flex Consumption function without fix:
Consumption function with fix:
Consumption function without fix:
I tested by deploying with serverless-compat-self-monitoring:
aas.environment.instance_namespan attribute populated