feat: add object form support for observability.otlp.headers#28524
feat: add object form support for observability.otlp.headers#28524
Conversation
- Change OTLPConfig.Headers from string to any to accept both string (deprecated) and map forms - Add normalizeOTLPHeaders helper to convert string/map to OTEL env var format - Emit deprecation warning when string form is used - Update extractOTLPConfigFromRaw and injectOTLPConfig to handle map form - Update mcp_gateway_config.go to normalize any headers - Update JSON schema to accept headers as string (deprecated) or object - Add comprehensive tests for new map form and normalizeOTLPHeaders" Agent-Logs-Url: https://github.com/github/gh-aw/sessions/2675d86c-ca85-4710-9264-b59e71b4b1b0 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Also add test for non-string map value skipping behavior Agent-Logs-Url: https://github.com/github/gh-aw/sessions/2675d86c-ca85-4710-9264-b59e71b4b1b0 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot when generating mask for headers, mask authorization value (remove Bearer as well) |
🧪 Test Quality Sentinel ReportTest Quality Score: 83/100✅ Excellent test quality
Test Classification DetailsView All Test Classifications (18 sub-cases across 4 functions)
Minor Observationsi️ Test Inflation (4.2:1 ratio)The test file grew 258 lines vs. 62 production lines. This mechanically triggers the inflation flag, but in this case the ratio is justified: the new feature supports two header formats (string and i️ One assertion missing message (
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Test Quality Sentinel: 83/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). All 4 new test functions verify behavioral contracts across multiple input paths (nil, string, map, empty, unsupported types). One minor: a single assert.Contains call is missing a descriptive message argument.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Commit pushed:
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (341 new lines in AI has analyzed the PR diff and generated a draft ADR to help you get started: 📄 Draft ADR: What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Draft ADR summaryDecision: Extend Key trade-offs captured:
📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
References: §24943747500
|
There was a problem hiding this comment.
Pull request overview
Adds support for configuring observability.otlp.headers as a map/object in workflow frontmatter (while keeping the legacy string form working but deprecated), and normalizes both forms into the key=value,key=value format required by OTEL_EXPORTER_OTLP_HEADERS.
Changes:
- Change
OTLPConfig.Headerstoanyand addnormalizeOTLPHeadersto accept either string or map forms (with deterministic ordering for maps). - Emit a deprecation warning to stderr when the legacy string form is used.
- Update JSON schema, docs, and add/extend tests to cover map-form headers behavior.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/observability_otlp.go | Adds normalizeOTLPHeaders and uses it during raw extraction + env injection; prints deprecation warnings for string form. |
| pkg/workflow/observability_otlp_test.go | Updates parsing assertions to normalize headers; adds new tests for normalization, injection, and extraction with map headers. |
| pkg/workflow/mcp_gateway_config.go | Normalizes headers when building MCP gateway runtime config. |
| pkg/workflow/frontmatter_types.go | Updates OTLP headers type/docs to support both map and deprecated string forms. |
| pkg/parser/schemas/main_workflow_schema.json | Allows observability.otlp.headers to be either an object (string values) or a deprecated string. |
| docs/src/content/docs/reference/frontmatter-full.md | Documents the new oneOf formats for observability.otlp.headers. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 7/7 changed files
- Comments generated: 2
| normalized, deprecated := normalizeOTLPHeaders(raw) | ||
| if deprecated { | ||
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage( | ||
| "observability.otlp.headers: string form is deprecated. Use the map form instead (e.g. headers: {Authorization: \"Bearer ${{ secrets.TOKEN }}\"})", | ||
| )) |
There was a problem hiding this comment.
extractOTLPConfigFromRaw now emits a deprecation warning to stderr when the string form is encountered. This function is called from multiple places (e.g., injectOTLPConfig and buildMCPGatewayConfig), so the same workflow can end up printing the warning multiple times. It also adds an unconditional stderr side effect to what was previously a pure extraction helper (which will make unit test runs noisy). Consider returning the deprecation flag (or an enum) from extractOTLPConfigFromRaw and emitting the warning once at a higher level (or adding an option to suppress/warn-once).
| @@ -141,7 +141,8 @@ func buildMCPGatewayConfig(workflowData *WorkflowData) *MCPGatewayRuntimeConfig | |||
| if otlpHeaders == "" && workflowData.ParsedFrontmatter != nil && | |||
| workflowData.ParsedFrontmatter.Observability != nil && | |||
| workflowData.ParsedFrontmatter.Observability.OTLP != nil { | |||
| otlpHeaders = workflowData.ParsedFrontmatter.Observability.OTLP.Headers | |||
| normalized, _ := normalizeOTLPHeaders(workflowData.ParsedFrontmatter.Observability.OTLP.Headers) | |||
There was a problem hiding this comment.
buildMCPGatewayConfig re-reads OTLP headers via extractOTLPConfigFromRaw, which now prints the deprecation warning. Since injectOTLPConfig also calls extractOTLPConfigFromRaw during compilation, this can cause duplicate warnings for a single workflow. Additionally, the ParsedFrontmatter fallback ignores the returned deprecated flag, so if headers only come from ParsedFrontmatter (e.g., via imports) this path won’t emit the deprecation warning unless another stage already did. Consider sourcing the normalized headers from a single place (e.g., store normalized headers on WorkflowData alongside OTLPEndpoint) and emitting the warning once (warn-once).
…ask step The generateOTLPHeadersMaskStep now: 1. Masks the entire OTEL_EXPORTER_OTLP_HEADERS value (existing) 2. Parses each comma-separated header pair and masks the individual value 3. For "Bearer <token>" Authorization values, also masks the raw token without the scheme prefix so it's redacted even when it appears alone Agent-Logs-Url: https://github.com/github/gh-aw/sessions/f0388cf2-5cdf-4713-9c48-034efe1230db Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in c65a738. The mask step now:
|
|
@copilot review all comments |
…nings Agent-Logs-Url: https://github.com/github/gh-aw/sessions/9d7b90fb-97a7-4d37-81ea-71daf6c9e0e5 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…Headers - extractOTLPConfigFromRaw now returns a deprecated bool flag instead of printing the warning inline (pure helper, no side-effects) - injectOTLPConfig collects the deprecated flag from both extraction paths (raw frontmatter and ParsedFrontmatter fallback) and emits the warning exactly once - Add WorkflowData.OTLPHeaders field; injectOTLPConfig sets it alongside OTLPEndpoint so downstream code has a single source of truth - buildMCPGatewayConfig now reads workflowData.OTLPHeaders directly, eliminating the second call to extractOTLPConfigFromRaw and the duplicate deprecation warning it caused - Update tests: extend TestExtractOTLPConfigFromRaw to assert the deprecated return value; add TestInjectOTLPConfig_OTLPHeadersField Agent-Logs-Url: https://github.com/github/gh-aw/sessions/9d7b90fb-97a7-4d37-81ea-71daf6c9e0e5 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed both reviewer comments in a7a7888:
|
|
@copilot merge main, fix conflicts and recompile |
…bject-form-otlp-headers # Conflicts: # .github/workflows/daily-fact.lock.yml # .github/workflows/smoke-codex.lock.yml Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
|
|
|
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
🚀 Smoke OpenCode MISSION COMPLETE! OpenCode delivered. 🔥 |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
Agent Container Tool Check
Result: 12/12 tools available ✅ Overall Status: PASS
|
|
Smoke Test Codex 24945190968: FAIL Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "ab.chatgpt.com"
- "chatgpt.com"See Network Configuration for more information.
|
|
🤖 Smoke test complete for PR #28524
Overall: ✅ PASS Author:
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
Smoke test review: PR adds object form support for OTLP headers. Implementation looks solid with proper deprecation handling and comprehensive test coverage.
📰 BREAKING: Report filed by Smoke Copilot · ● 1.1M
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
Commit pushed:
|
|
Smoke Test Run §24945190957 — Overall: PARTIAL Core (#1–12): ✅✅✅✅✅✅✅✅✅❌✅✅ | PR Review (#13–19): ✅✅✅
|
There was a problem hiding this comment.
💥 Automated smoke test review — all systems nominal! Run 24945190957.
💥 [THE END] — Illustrated by Smoke Claude · ● 190.1K
| job-name: ${{ github.job }} | ||
| - name: Mask OTLP telemetry headers | ||
| run: echo '::add-mask::'"$OTEL_EXPORTER_OTLP_HEADERS" | ||
| run: bash "${RUNNER_TEMP}/gh-aw/actions/mask_otlp_headers.sh" |
There was a problem hiding this comment.
🤖 Smoke test review comment #1 — Good refactor! Using a dedicated shell script mask_otlp_headers.sh instead of an inline echo command improves maintainability and allows the masking logic to evolve independently of the lock file generation. Run 24945190957.
| job-name: ${{ github.job }} | ||
| - name: Mask OTLP telemetry headers | ||
| run: echo '::add-mask::'"$OTEL_EXPORTER_OTLP_HEADERS" | ||
| run: bash "${RUNNER_TEMP}/gh-aw/actions/mask_otlp_headers.sh" |
There was a problem hiding this comment.
🤖 Smoke test review comment #2 — Consistent pattern across all 202 lock files. Centralizing the OTLP header masking in mask_otlp_headers.sh means future improvements only need a single edit. Run 24945190957.
- audit.md: document tokens-per-turn and tool calls/bash command breakdown in diff output (PR #28494) - frontmatter.md: add Observability section documenting observability.otlp configuration, object-form OTLP headers (PR #28524), and gen_ai.* span attribute conventions replacing gh-aw.* (PR #28511) - cli.md: update audit diff description to include new output fields (PR #28494) - cache-memory.md: document cache path misconfiguration detection via missing_data signal (PR #28516) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
Adds support for using a map/object form for the
observability.otlp.headersfield in workflow frontmatter, similar to howenvfields work. This makes it cleaner and easier to set header secrets individually.The string form is now deprecated with a warning recommending the map form.
New map form (preferred)
Old string form (deprecated)
Changes
OTLPConfig.Headerschanged fromstringtoanyto accept both formsnormalizeOTLPHeadershelper converts either form to thekey=value,key=valueformat required byOTEL_EXPORTER_OTLP_HEADERSextractOTLPConfigFromRawis now a pure helper returning adeprecated boolflag; the warning is emitted at a single call site ininjectOTLPConfigWorkflowData.OTLPHeadersfield added (set byinjectOTLPConfigalongsideOTLPEndpoint) so downstream code (buildMCPGatewayConfig) reads it directly instead of re-reading raw frontmatter, eliminating duplicate warningsheadersas either string (deprecated) or object with string valuesBearerAuthorization values — also the raw token after stripping the `****** scheme prefix so the token is redacted even when it appears without the schemedeprecatedflag assertions andOTLPHeadersfield coverageChangeset
observability.otlp.headersas an object in workflow frontmatter.Warning
Firewall blocked 2 domains
The following domains were blocked by the firewall during workflow execution:
ab.chatgpt.comchatgpt.comSee Network Configuration for more information.
✨ PR Review Safe Output Test - Run 24945190957