feat: wire analysis_type, tags, and notify into pick_experiment runtime#32534
feat: wire analysis_type, tags, and notify into pick_experiment runtime#32534Copilot wants to merge 7 commits into
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Wires the previously-declared but unused analysis_type, tags, and notify fields of ExperimentConfig into the runtime of pick_experiment, surfacing them in the GitHub Actions step summary and in OTEL_RESOURCE_ATTRIBUTES, and emitting a core.notice marker when all variants reach min_samples.
Changes:
- Render
Analysis typeandTagsin the Experiment Details section of the step summary, and add a 🔔 notification line whenallReadyandnotify.discussion/notify.issueis configured. - Emit
experiment.<name>.analysis_type=...andexperiment.<name>.tag.<tag>=trueresource attributes alongside the existing variant attribute, and callcore.notice("EXPERIMENT_READY: ...")on readiness. - Add 11 new unit tests covering summary rendering, OTEL attribute inclusion/omission, and
core.noticefiring/non-firing.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/pick_experiment.cjs | Adds analysis_type/tags rendering, notification summary line + core.notice, and extends OTEL_RESOURCE_ATTRIBUTES with analysis_type and per-tag attributes. |
| actions/setup/js/pick_experiment.test.cjs | Adds notice to mockCore and 11 new tests for the new summary, OTEL, and notice behaviours. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 2
| const tagAttrs = Object.entries(configs) | ||
| .filter(([, cfg]) => cfg.tags?.length) | ||
| .flatMap(([name, cfg]) => (cfg.tags ?? []).map(t => `experiment.${name}.tag.${t}=true`)) | ||
| .join(","); |
| @@ -285,6 +295,10 @@ async function writeSummary(assignments, configs, state, core) { | |||
| lines.push(""); | |||
| lines.push(`**Hypothesis:** ${hypothesis}`); | |||
| } | |||
| if (cfg?.analysis_type) { | |||
| lines.push(""); | |||
| lines.push(`**Analysis type:** \`${cfg.analysis_type}\``); | |||
| } | |||
| if (guardrails && guardrails.length > 0) { | |||
| lines.push(""); | |||
| lines.push("**Guardrail metrics:**"); | |||
| @@ -300,6 +314,11 @@ async function writeSummary(assignments, configs, state, core) { | |||
| lines.push(`Tracking issue: #${issue}`); | |||
| } | |||
| } | |||
| const tags = cfg?.tags; | |||
| if (tags && tags.length > 0) { | |||
| lines.push(""); | |||
| lines.push(`**Tags:** ${tags.map(t => `\`${t}\``).join(", ")}`); | |||
| } | |||
|
@copilot review all comments |
…n summary Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed both reviewer findings:
Both fixed in commit |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Three fields declared in
ExperimentConfig(analysis_type,tags,notify) were fully typed but had no runtime effect. This wires all three into the step summary and OTEL telemetry.analysis_type**Analysis type:** \mann_whitney``experiment.<name>.analysis_type=<value>inOTEL_RESOURCE_ATTRIBUTEStags**Tags:** \team:infra`, `q1-2026``experiment.<name>.tag.<tag>=trueOTEL resource attributesnotifyWhen all variants reach
min_samples(allReady):core.notice("EXPERIMENT_READY: name=<n> discussion=<N>")/issue=<N>— machine-readable marker for downstream notification workflows🔔 Notification will be sent to discussion/issue #Nline in the step summary for bothnotify.discussionandnotify.issueTests (
pick_experiment.test.cjs)notice: vi.fn()tomockCorecore.noticefiring onallReady, and the negative case (notice not emitted belowmin_samples)