Repository navigation
Reuse workflow metadata in OTLP attributes - #70860
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Namespace detection rejects valid literal attributes and unrelated expressions, breaking existing workflow compatibility.
1 open issue
What changed in this PR
Adds opt-in reuse of root workflow metadata in gh-aw’s OTLP telemetry, including shared imports.
Changes:
- Resolves whole-value metadata references at compile time.
- Preserves imported resource attributes and existing precedence.
- Adds tests, reference documentation, and a patch release note.
| File | Description |
|---|---|
pkg/workflow/observability_metadata.go |
Resolves and validates metadata references. |
pkg/workflow/observability_metadata_test.go |
Tests resolution and rejection cases. |
pkg/workflow/compiler_orchestrator_workflow.go |
Runs resolution after import merging. |
pkg/workflow/compiler_orchestrator_workflow_test.go |
Tests merged attribute resolution and emission. |
pkg/parser/import_observability.go |
Preserves imported resource attributes. |
pkg/parser/import_observability_test.go |
Tests first-import precedence. |
docs/src/content/docs/reference/open-telemetry.mdx |
Documents metadata references in both attribute maps. |
docs/src/content/docs/reference/imports.md |
Explains root-scoped references in shared imports. |
docs/src/content/docs/reference/frontmatter.md |
Documents opt-in metadata reuse. |
.changeset/otlp-metadata-attributes.md |
Records the patch release change. |
Review effort: Max
| } | ||
| for name, value := range attributes { | ||
| text, ok := value.(string) | ||
| if !ok || !strings.Contains(text, otlpMetadataNamespace) { |
There was a problem hiding this comment.
Fixed in 7176f09: detection now scans actual expression references outside quoted strings, ignores literal mentions, and rejects bracket-selected metadata access in both attribute maps.
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Commit pushed:
|
|
✅ Ponytail Reviewer completed successfully!
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
L51-54: shrink: manual extraction repeats the validation regex. Use FindStringSubmatch(text)[1] after the match check.
net: -3 lines possible.
Generated by ✂️ Ponytail Reviewer for #70860 · codex · gpt56 · 7.79 AIC · ⌖ 5.42 AIC · ⊞ 14.8K
Comment /ponytail to run again
| if !otlpMetadataReferencePattern.MatchString(text) { | ||
| return fmt.Errorf("%s must be a whole-value reference in the form ${{ github.aw.metadata.<key> }}", scope) | ||
| } | ||
| key := strings.TrimSpace(text) |
There was a problem hiding this comment.
L51-54: shrink: manual extraction repeats the validation regex. Use FindStringSubmatch(text)[1] after the match check.
There was a problem hiding this comment.
Fixed in 7176f09: the metadata key now comes from the validated FindStringSubmatch capture instead of being manually extracted.
There was a problem hiding this comment.
Review summary
No additional actionable issues were identified in the changed lines beyond the existing review comment on pkg/workflow/observability_metadata.go:44. The implementation and accompanying tests appear internally consistent; static review only, no tests executed.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · 2.17 AIC · ⌖ 0.548 AIC · ⊞ 1.6K
Comment /review to run again
🏗️ Design Decision Gate — ADR RequiredThis PR adds 328 new lines of business logic under 📄 Draft ADR committed: Decision captured in the draft: At compile time, resolve whole-value
📋 What to do next
Once the ADR is linked in the PR body, this gate re-runs and checks that the implementation matches the decision. ❓ Why ADRs MatterADRs keep a searchable, permanent record of why the codebase looks the way it does. Future contributors, including your future self, will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR is complete only when it has these four sections:
ADRs live in
|
|
@copilot update compiler threat model and add a actions/aw linter to detect leaks in otlp messages |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnosing-bugs (per pr-triage) — commenting with test-coverage and robustness suggestions on the new resolveOTLPMetadataReferences logic.
📋 Key Themes & Highlights
Key Themes
- Determinism: iterating a
map[string]anywhile short-circuiting on the first validation error means the reported error can vary by compile run when multiple attributes are invalid. - Guard precision: the
otlpMetadataNamespacesubstring check (flagged independently by an existing reviewer comment) can misclassify literal strings or unrelated expressions containing the namespace text as broken metadata references. - Test gap: the
map[string]stringbranch ofnormalizeOTLPAttributeMap— the exact shape produced by the import-merge path just before this function runs — isn't directly exercised by a test.
Positive Highlights
- ✅ Solid security posture: rejects nested selectors, embedded references, non-string metadata, and metadata values containing expression delimiters, with dedicated regression tests for each.
- ✅ Good integration test (
TestMergeImportedObservabilityResolvesRootMetadataReferences) verifying the full compiler pipeline end-to-end, including env propagation. - ✅ Clear, well-scoped error messages that name the exact frontmatter path (
observability.otlp.<field>.<name>). - ✅ Documentation updates across frontmatter, imports, and open-telemetry reference pages are consistent and accurate.
Investigation note: Review was bounded to the pre-fetched diff and the two core files (observability_metadata.go, compiler_orchestrator_workflow.go); no uncertainty beyond the three issues flagged inline.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 125.8 AIC · ⌖ 15.2 AIC · ⊞ 10.2K
Comment /matt to run again
| if attributes == nil { | ||
| continue | ||
| } | ||
| for name, value := range attributes { |
There was a problem hiding this comment.
[/diagnosing-bugs] Iterating attributes (a map[string]any) to find a validation error means that when multiple attributes are invalid, Go's randomized map iteration order makes which error message is actually returned non-deterministic across compiles. That hurts reproducibility when a user reports a compile failure and CI/local runs surface a different offending key.
💡 Suggested fix
Collect names into a sorted slice before iterating, so error reporting (and therefore debugging) is stable:
names := make([]string, 0, len(attributes))
for name := range attributes {
names = append(names, name)
}
sort.Strings(names)
for _, name := range names {
value := attributes[name]
// ... existing validation ...
}This also makes future regression tests for "multiple invalid attributes" scenarios deterministic.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 7176f09: attribute keys are sorted before validation so the first reported error is deterministic.
| } | ||
| for name, value := range attributes { | ||
| text, ok := value.(string) | ||
| if !ok || !strings.Contains(text, otlpMetadataNamespace) { |
There was a problem hiding this comment.
[/tdd] Agreeing with the existing reviewer comment here: the guard strings.Contains(text, otlpMetadataNamespace) is a substring check against the literal text github.aw.metadata, not an expression-boundary check. A value like the literal string "github.aw.metadata.version" (no ${{ }}) or an unrelated expression such as ${{ contains(github.aw.metadata.version, x) }} would hit this branch and then fail the whole-value regex with a confusing error, even though the author never intended a metadata reference.
💡 Suggested test + fix
Add a test case with a literal string value containing the substring but not wrapped as ${{ ... }}, asserting it passes through unchanged (or is handled with a clearer, more specific error). Consider checking strings.Contains(text, "${{") combined with the namespace substring before entering this branch, so truly unrelated values are skipped rather than misclassified as broken metadata references.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 7176f09: literal namespace text and quoted expression mentions are ignored; actual metadata accesses inside expressions are still validated.
| switch attributes := otlp[field].(type) { | ||
| case map[string]any: | ||
| return attributes | ||
| case map[string]string: |
There was a problem hiding this comment.
[/tdd] The map[string]string branch of normalizeOTLPAttributeMap (triggered when otlp.attributes/otlp.resource-attributes is already typed as map[string]string, e.g. the output of mergeOTLPStringMaps in the import-merge path) has no direct test exercising it — observability_metadata_test.go only constructs frontmatter with map[string]any. Since this is the exact shape produced right before this function runs in mergeImportedWorkflowConfiguration, add a test that sets up RawFrontmatter with a pre-merged map[string]string attributes map to confirm the mutation-and-normalize path works correctly end-to-end.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 7176f09: added a direct test with both pre-merged map[string]string attribute maps in RawFrontmatter, confirming they normalize and resolve.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added the CTR-017 OTLP leak lint rule and updated the compiler threat model/docs in commit |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot address the following outstanding work in one pass:
Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI. Sous-chef head: 8ca0d9e
|
…-workflow-metadata Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
OTLP custom attributes could not reuse workflow metadata, and shared imports dropped resource attributes. This adds compile-time, root-scoped metadata references to both custom attribute maps.
${{ github.aw.metadata.<key> }}references inobservability.otlp.attributesandresource-attributes. Missing keys, unsupported selectors, embedded references, and metadata containing GitHub Actions expression delimiters fail compilation.