fix: respect user-defined OTEL_SERVICE_NAME when OTLP observability is enabled#46779
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #46779 does not have the 'implementation' label and has only 26 new lines of code in business logic directories (threshold: 100). |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Prevents duplicate OTEL_SERVICE_NAME keys when OTLP observability is enabled.
Changes:
- Preserves user-defined service names.
- Adds regression coverage for duplicate-key prevention.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/observability_otlp.go |
Conditionally injects the service name. |
pkg/workflow/observability_otlp_test.go |
Tests user-defined service-name preservation. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Medium
|
|
||
| var sentryEndpointExpressionPattern = regexp.MustCompile(`(?i)^\$\{\{\s*secrets\.` + regexp.QuoteMeta(constants.OTELSentryEndpointSecretName) + `\s*\}\}$`) | ||
| var otlpResourceAttributeSecretRefPattern = regexp.MustCompile(`\$\{\{\s*(secrets|vars)\.`) | ||
| var otelServiceNameKeyPattern = regexp.MustCompile(`(?m)^\s*OTEL_SERVICE_NAME:`) |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix is correct and well-scoped. The multiline regex (?m)^\s*OTEL_SERVICE_NAME: reliably detects an existing user-defined key (comment lines beginning with # are not matched since # is not \s). The new test case covers preservation of user-defined values and the no-duplicate invariant. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 14 AIC · ⌖ 4.31 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — approving with one minor test-coverage suggestion.
📋 Key Themes & Highlights
Findings
- The test asserts the user value is preserved and not duplicated ✅ — but does not assert
OTEL_RESOURCE_ATTRIBUTESis still injected in the same code path (see inline comment)
Positive Highlights
- ✅ Root cause is properly addressed: duplicate key avoided by checking before injecting, not patching after
- ✅ Regex
(?m)^\s*OTEL_SERVICE_NAME:correctly ignores commented-out keys and value occurrences - ✅ Regression test is well-structured and readable
- ✅ Minimal, surgical change — no collateral modifications
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 16.7 AIC · ⌖ 4.47 AIC · ⊞ 6.7K
Comment /matt to run again
| assert.Equal(t, 1, strings.Count(wd.Env, "OTEL_SERVICE_NAME:"), "OTEL_SERVICE_NAME should appear exactly once") | ||
| assert.Contains(t, wd.Env, "OTEL_EXPORTER_OTLP_ENDPOINT: https://traces.example.com", "endpoint should still be injected") | ||
| }) | ||
|
|
There was a problem hiding this comment.
[/tdd] The test verifies OTEL_SERVICE_NAME is preserved and not duplicated, but doesn't assert that OTEL_RESOURCE_ATTRIBUTES is still injected when the user supplies their own service name.
💡 Suggested addition
assert.Contains(t, wd.Env, "OTEL_RESOURCE_ATTRIBUTES:", "resource attributes should still be injected when user defines OTEL_SERVICE_NAME")This guards against a future regression where skipping OTEL_SERVICE_NAME injection accidentally skips the OTEL_RESOURCE_ATTRIBUTES line too. The current code is correct, but a test assertion makes the invariant explicit.
@copilot please address this.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics (1 test)
Verdict
Quality notes:
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
There was a problem hiding this comment.
Changes requested — one medium-severity correctness issue must be addressed
The fix is sound in intent, but the regex-based detection method has a correctness gap.
Blocking issue summary
The pattern (?m)^\s*OTEL_SERVICE_NAME: is too broad — it matches any line beginning with optional whitespace followed by that key, including lines embedded inside YAML block scalar values. A user env block like:
env:
SOME_TEMPLATE: |
OTEL_SERVICE_NAME: placeholderwould silently suppress injection of the real OTEL_SERVICE_NAME mapping key, leaving telemetry un-tagged with no error or warning. The test covers only the straightforward case and misses this edge.
Minimum fix: tighten the pattern to (?m)^ OTEL_SERVICE_NAME: (exactly two leading spaces matching the env block indentation convention). Preferred fix: parse the env block as YAML and check keys structurally.
🔎 Code quality review by PR Code Quality Reviewer · 47.1 AIC · ⌖ 4.67 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
pkg/workflow/observability_otlp.go:9
False-positive risk: regex fires inside YAML multiline string values, not just mapping keys.
<details>
<summary>💡 Details and suggested fix</summary>
The pattern (?m)^\s*OTEL_SERVICE_NAME: uses multiline mode, meaning ^ matches the start of any embedded line — including lines inside block scalar values. For example, this env block would incorrectly suppress injection:
env:
SOME_TEMPLATE: |
OTEL_SERVICE_NAME: placeholderThe match would fire on the indented `OTEL_S…
pkg/workflow/observability_otlp.go:22
Printf with no format arguments — use Print to avoid accidental misinterpretation of % characters.
<details>
<summary>💡 Suggested fix</summary>
// before
otlpLog.Printf("Skipping OTEL_SERVICE_NAME injection: already defined by user")
// after
otlpLog.Print("Skipping OTEL_SERVICE_NAME injection: already defined by user")Using Printf with a string that contains no format verbs compiles today but is a latent bug: if the message is ever extended to interpolate e.g. `serviceN…
|
@copilot add a fallback env var with the expression that can be used in JavaScript if the model expression resolves to empty. That way it handles any syntax. |
|
Great work! 🎯 This fix elegantly solves the duplicate Key strengths:
This PR is ready for review and merge! Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "patchdiff.githubusercontent.com"See Network Configuration for more information.
|
|
🎉 This pull request is included in a new release. Release: |
When a user sets
OTEL_SERVICE_NAMEin their workflowenv:block alongsideobservability.otlp, the compiler unconditionally appended its ownOTEL_SERVICE_NAMEentry, producing a duplicate YAML key and failing compilation.Changes
pkg/workflow/observability_otlp.go: Skip injectingOTEL_SERVICE_NAMEwhen the key is already present inworkflowData.Env. Detection uses a compiled multiline regex ((?m)^\s*OTEL_SERVICE_NAME:) anchored to line-start to avoid false positives from comments or values that happen to contain the key name.pkg/workflow/observability_otlp_test.go: New test case asserting that a user-definedOTEL_SERVICE_NAMEis preserved verbatim and appears exactly once after injection.Example
Previously this workflow would fail to compile:
After this change the user-supplied value is honoured and no duplicate is emitted.