Conversation
…lows Agent-Logs-Url: https://github.com/github/gh-aw/sessions/c8b78e5c-b8d6-41f8-b2b6-ec03d42c4e89 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
pelikhan
left a comment
pelikhan
left a comment
There was a problem hiding this comment.
@copilot create a helper file, tlp.cgs, that users can require. It should provide a stable, service-level API for OpenTelemetry with minimal configuration compared to the samples here. The user should be able to log attributes with a tool name, and the library should automatically handle environment variables and all underlying machinery. Users shouldn’t need to manage any of that. The goal is to expose a very small public function surface while keeping the internals flexible so they can be refactored over time.
…s guide Agent-Logs-Url: https://github.com/github/gh-aw/sessions/b39f1de9-8317-42a1-af11-3ac6661f7e12 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in commit 927c3f6. Added |
|
@copilot Require shims.cjs and otlp.cjs to get the shims for core and other action-specific JavaScript. |
…ontext are available Agent-Logs-Url: https://github.com/github/gh-aw/sessions/048292c0-1229-4789-a893-5e967b10c76f Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in commit ddbf503. |
| // --------------------------------------------------------------------------- | ||
|
|
||
| /** @type {typeof import('./send_otlp_span.cjs') | null} */ | ||
| let _core = null; |
There was a problem hiding this comment.
@copilot first, core is a reserved namespace and should not be used. Second, do not capture the result of a call to require; it is already cached by Node.
There was a problem hiding this comment.
Fixed in commit 39dfff0. Removed the _core variable and the core() helper function entirely. logSpan now calls require(path.join(__dirname, "send_otlp_span.cjs")) inline — Node's module cache ensures the module is only evaluated once regardless of how many times require is called.
…stead Agent-Logs-Url: https://github.com/github/gh-aw/sessions/ed5944ce-df74-4de6-a600-01a233a15173 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🧪 Test Quality Sentinel ReportTest Quality Score: 81/100✅ Excellent test quality
Test Classification DetailsView All 14 Test Classifications
Flagged Tests — Requires ReviewNo tests flagged. All tests enforce behavioral contracts of the Note on mocking strategy: The tests patch Language SupportTests analyzed:
Score Breakdown
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds a stable JavaScript helper (otlp.cjs) plus documentation describing how shared agentic workflow imports can emit custom OTLP spans/attributes correlated to the same distributed trace as gh-aw’s built-in instrumentation.
Changes:
- Introduces
actions/setup/js/otlp.cjswith a singlelogSpan(toolName, attributes, options)API that reads trace context + endpoint from env and is non-fatal on export failures. - Adds
actions/setup/js/otlp.test.cjsunit tests forlogSpanbehavior (trace/parent handling, attribute conversion, status, overrides, resilience, shim integration). - Adds a new documentation guide and links it in the Guides sidebar.
Show a summary per file
| File | Description |
|---|---|
| docs/src/content/docs/guides/custom-otlp-attributes.md | New guide explaining how to emit custom OTLP spans/attributes from shared workflow imports. |
| docs/astro.config.mjs | Adds the new guide to the Guides sidebar. |
| actions/setup/js/otlp.cjs | New high-level OTLP helper wrapping send_otlp_span.cjs with a minimal public API. |
| actions/setup/js/otlp.test.cjs | New Vitest unit tests covering the helper’s key behaviors and failure handling. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
actions/setup/js/otlp.cjs:86
- When
OTEL_EXPORTER_OTLP_ENDPOINTis unset,endpointbecomes an empty string andsendOTLPSpanwill still attempt a fetch to"/v1/traces"(which will warn/retry with an invalid URL). If the goal is “debugging without a live collector”, consider short-circuiting whenendpointis empty: still write the JSONL mirror, but skip the HTTP export entirely to avoid noisy warnings and retry delays.
const endpoint = options.endpoint ?? process.env.OTEL_EXPORTER_OTLP_ENDPOINT ?? "";
const traceId = options.traceId ?? process.env.GITHUB_AW_OTEL_TRACE_ID ?? "";
const parentSpanId = options.parentSpanId ?? process.env.GITHUB_AW_OTEL_PARENT_SPAN_ID ?? "";
if (!isValidTraceId(traceId)) {
console.warn(`[otlp] ${toolName}: GITHUB_AW_OTEL_TRACE_ID is not set or invalid; skipping span`);
docs/src/content/docs/guides/custom-otlp-attributes.md:116
- The guide says
/tmp/gh-aw/otel.jsonlis included in thefirewall-audit-logsartifact and suggestsgh aw logs <run-id> --artifacts firewall, but the artifacts reference showsfirewall-audit-logscontains firewall/audit logs only (nootel.jsonl). The OTLP mirror is part of the unifiedagentartifact when OTLP is enabled, so the download instructions/artifact name here should be updated accordingly (and note where the file lands under the logs output directory).
Every span is always appended as a JSON line to `/tmp/gh-aw/otel.jsonl`, even when `OTEL_EXPORTER_OTLP_ENDPOINT` is not set. This file is included in the `firewall-audit-logs` artifact so you can inspect spans after the run:
```bash
# Download firewall/telemetry artifacts for a run
gh aw logs <run-id> --artifacts firewall
# Inspect spans emitted by your tool
cat otel.jsonl | jq 'select(.resourceSpans[].scopeSpans[].spans[].name | startswith("my-tool"))'
</details>
- **Files reviewed:** 4/4 changed files
- **Comments generated:** 3
| All attribute values are sanitized before the payload is sent over the wire: | ||
|
|
||
| - **Redacts** the value of any attribute whose key matches `token`, `secret`, `password`, `passwd`, `key`, `auth`, `credential`, `api-key`, or `access-key` (case-insensitive), replacing it with `[REDACTED]`. | ||
| - **Truncates** string values longer than 1,024 characters. | ||
|
|
||
| Sanitization is automatic — you do not need to call it yourself. | ||
|
|
||
| ## Debugging without a live collector | ||
|
|
||
| Every span is always appended as a JSON line to `/tmp/gh-aw/otel.jsonl`, even when `OTEL_EXPORTER_OTLP_ENDPOINT` is not set. This file is included in the `firewall-audit-logs` artifact so you can inspect spans after the run: |
There was a problem hiding this comment.
This section implies sensitive attribute redaction/truncation is automatic, but the local JSONL mirror (otel.jsonl) is currently written with the raw payload (before sanitizeOTLPPayload is applied) in send_otlp_span.cjs. That means secrets can still end up in the artifact even though the over-the-wire export is sanitized. Either clarify in the docs that the JSONL mirror is not sanitized, or adjust otlp.logSpan to write a sanitized copy to the mirror (e.g., use sanitizeOTLPPayload + appendToOTLPJSONL and call sendOTLPSpan with skipJSONL: true).
This issue also appears on line 108 of the same file.
| All attribute values are sanitized before the payload is sent over the wire: | |
| - **Redacts** the value of any attribute whose key matches `token`, `secret`, `password`, `passwd`, `key`, `auth`, `credential`, `api-key`, or `access-key` (case-insensitive), replacing it with `[REDACTED]`. | |
| - **Truncates** string values longer than 1,024 characters. | |
| Sanitization is automatic — you do not need to call it yourself. | |
| ## Debugging without a live collector | |
| Every span is always appended as a JSON line to `/tmp/gh-aw/otel.jsonl`, even when `OTEL_EXPORTER_OTLP_ENDPOINT` is not set. This file is included in the `firewall-audit-logs` artifact so you can inspect spans after the run: | |
| Attribute values are sanitized automatically before the OTLP payload is sent over the wire: | |
| - **Redacts** the value of any attribute whose key matches `token`, `secret`, `password`, `passwd`, `key`, `auth`, `credential`, `api-key`, or `access-key` (case-insensitive), replacing it with `[REDACTED]`. | |
| - **Truncates** string values longer than 1,024 characters. | |
| You do not need to call sanitization yourself for OTLP export. However, if you use the local JSONL mirror for debugging, do not assume it is redacted — avoid putting secrets or other sensitive values into custom attributes. | |
| ## Debugging without a live collector | |
| Every span is always appended as a JSON line to `/tmp/gh-aw/otel.jsonl`, even when `OTEL_EXPORTER_OTLP_ENDPOINT` is not set. This local debug file is included in the `firewall-audit-logs` artifact so you can inspect spans after the run, but it should be treated as potentially containing raw attribute values rather than a sanitized/redacted copy: |
| const { buildAttr, buildOTLPPayload, sendOTLPSpan, generateSpanId, isValidTraceId, isValidSpanId, SPAN_KIND_CLIENT } = require(path.join(__dirname, "send_otlp_span.cjs")); | ||
|
|
||
| const now = Date.now(); | ||
| const startMs = options.startMs ?? now; | ||
| const endMs = options.endMs ?? now; | ||
|
|
||
| const endpoint = options.endpoint ?? process.env.OTEL_EXPORTER_OTLP_ENDPOINT ?? ""; | ||
| const traceId = options.traceId ?? process.env.GITHUB_AW_OTEL_TRACE_ID ?? ""; | ||
| const parentSpanId = options.parentSpanId ?? process.env.GITHUB_AW_OTEL_PARENT_SPAN_ID ?? ""; | ||
|
|
||
| if (!isValidTraceId(traceId)) { | ||
| console.warn(`[otlp] ${toolName}: GITHUB_AW_OTEL_TRACE_ID is not set or invalid; skipping span`); | ||
| return; | ||
| } | ||
|
|
||
| const spanAttrs = Object.entries(attributes).map(([k, v]) => buildAttr(k, v)); |
There was a problem hiding this comment.
sendOTLPSpan appends the raw payload to otel.jsonl before sanitization, so using it directly here can leak sensitive attributes into the local mirror artifact. If the intention is that otlp.logSpan is safe-by-default (as the docs suggest), consider mirroring a sanitized payload (via sanitizeOTLPPayload + appendToOTLPJSONL) and then calling sendOTLPSpan(..., { skipJSONL: true }) for the network export.
This issue also appears on line 81 of the same file.
|
|
||
| expect(mockSendOTLPSpan).toHaveBeenCalledWith("https://otel.example.com", expect.anything()); | ||
| }); | ||
|
|
There was a problem hiding this comment.
There’s no unit test covering the “endpoint not set” path. Since the guide explicitly calls out debugging without a live collector, it would be good to add a test that deletes OTEL_EXPORTER_OTLP_ENDPOINT and asserts the expected behavior (e.g., no HTTP export attempt / no warning spam, while still producing the JSONL mirror if that’s intended).
| it("does not attempt HTTP export when OTEL_EXPORTER_OTLP_ENDPOINT is not set", async () => { | |
| delete process.env.OTEL_EXPORTER_OTLP_ENDPOINT; | |
| await otlp.logSpan("my-scanner", {}); | |
| expect(mockSendOTLPSpan).not.toHaveBeenCalled(); | |
| }); |
|
@copilot secret redaction and sanitization should be applied to attribute values |
|
@copilot review all comments |
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/0b99b385-a7e4-4c5f-87e5-bda110b61540 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in commit fca2491. |
|
@copilot needs to silently skip if environement variables are missing |
… written; update docs Agent-Logs-Url: https://github.com/github/gh-aw/sessions/71150d30-6996-41ac-9437-d6062c8168cb Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in commit 36d5fe4. Addressed all open reviewer comments:
|
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/9ebf1e4e-dce4-44e6-9dcb-194b3f152d88 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot review and make sure docs are up to date. Use tech-writer agent |
|
@copilot instrument qmd.md with otlp for index size, hits |
…sing trace ID Agent-Logs-Url: https://github.com/github/gh-aw/sessions/03160233-1e86-4379-ae68-df738ddd0f87 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in commit 8c0c844. The only stale text was in the |
…hits Agent-Logs-Url: https://github.com/github/gh-aw/sessions/e70388f4-5e8e-49f7-babf-1405b37ecf31 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in commit 039a0e3. Added a Telemetry section to |
Adds a stable, service-level JavaScript helper (
otlp.cjs) and a new documentation guide that explains how shared agentic workflow imports can emit additional OpenTelemetry spans and attributes. This enables third-party tools (APM agents, scanners, data pipeline steps) to upload their own telemetry alongside gh-aw's built-in instrumentation, all correlated under the same distributed trace.Changes
actions/setup/js/otlp.cjs— new high-level helper exposing a singlelogSpan(toolName, attributes, options)function:OTEL_EXPORTER_OTLP_ENDPOINT,GITHUB_AW_OTEL_TRACE_ID, andGITHUB_AW_OTEL_PARENT_SPAN_IDfrom the environment automatically — no configuration needed by callersshim.cjsat load time soglobal.coreandglobal.contextare always available, whether the module is loaded insidegithub-scriptor as a plain Node.js processsend_otlp_span.cjsinternally, keeping the public surface minimal and stable across future refactorsconsole.warnand never throwGITHUB_AW_OTEL_TRACE_IDis missing or invalid — zero noise in environments where OTLP tracing is not configuredsanitizeOTLPPayloadbefore writing to the local JSONL mirror so sensitive attribute values are redacted in both the over-the-wire export and the debug artifactOTEL_EXPORTER_OTLP_ENDPOINTis not set; the sanitized JSONL mirror is always written regardlessactions/setup/js/otlp.test.cjs— 16 unit tests covering happy path, attribute conversion, status codes, options overrides, silent skip on missing trace ID, error resilience, absent parent span ID handling, shim integration, JSONL sanitization, and the "endpoint not set" path (no HTTP export attempted, JSONL still written)docs/src/content/docs/guides/custom-otlp-attributes.md— new guide covering:otlp.logSpan()from a shared import'ssteps:entrylogSpanAPI reference table/tmp/gh-aw/otel.jsonlJSONL mirror artifact (always sanitized)send_otlp_span.cjsAPI for multiple linked spans and resource attributesdocs/astro.config.mjs— adds "Custom OTLP Attributes" entry to the Guides sidebar sectiondocs/src/content/docs/reference/qmd.md— adds a Telemetry section with a shared import example that emitsqmd.index.size(documents in the qmd index) andqmd.search.hits(cumulative search result count) viaotlp.logSpan; also links the new Custom OTLP Attributes guide from the Related Documentation section