You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(telemetry): preserve controls and share request attribution - #1691
Centralize request attribution across the CLI, SDK, and helpers. GitHub Action runs carry integration and configured-provider attribution, while token totals remain unknown when measurements are missing. Preserve account allowance snapshots independently of token measurements.
Changes
Share request context while preserving caller metadata, literal dotted keys, selected-profile analytics settings, and OpenTelemetry configuration.
Forward context through scan patches and fresh/resumed Deep workers; attribute publication and embedding requests through their existing request paths.
Distinguish missing usage from measured zero, retain retry-thread associations, preserve unpriced token totals, and enforce budgets against measured subtotals.
Save allowance snapshots from owned sessions even when token receipts are absent or incomplete. Keep finalization pending until token receipts arrive, without inventing zero usage.
Reuse existing native configuration and test helpers, including coverage of the cheaperinference built-in provider and concurrent request contexts.
Testing
After integrating main at 06201edfa54a83fdfc3a9d0e28bfb019ce30b4c0:
Python scan usage suite: all 127 tests passed.
The eight focused SDK suites produced 363 passes and 3 platform skips. Three comparison assertions failed because of local macOS temporary-path aliases; all four variants passed when rerun with a canonical temporary path.
The MCP Deep Scan executor module passed, including fresh/resumed worker settings and telemetry inheritance.
All five required portable source checks passed in order, including all nine compatibility-checker tests.
Plugin build, SDK/MCP type checks, changed TypeScript formatting, Action build/type/distribution checks, and diff checks passed.
Fresh hosted CI and Codex/Copilot reviews are required on the integrated head before merge.
Risk and rollout
No new CLI flags, Action inputs, collectors, or export destinations. The Action retains analytics.enabled=false; request attribution remains separate from native analytics and explicitly configured OTel exporters. Provider tags describe wrapper configuration; native managed-provider overrides can select a different backend. Attribution does not establish OpenAI-side reporting for other providers.
Migration 51 adds a local worker-thread registry, backfills known associations, and retains them until the scan is deleted. Allowance-only records retain unavailable token coverage and preserve existing legacy cost values.
Public disclosure review
No customer, partner, prospect, or user identities, data, or identifying details are included.
No credentials, personal data, private source, scan findings, or nonpublic links or tickets are included.
I reviewed the branch name, title, description, commits, changes, comments, logs, screenshots, attachments, and links for public disclosure.
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 454fc60. The implementation is reasonable, and I found no concrete correctness issue.
Nonblocking scope recommendation: separate request attribution and analytics/OTel propagation from usage/cost accounting, budget behavior, and retry-thread history (including migration 50). These are independently shippable changes; separating them would make the accounting and persistence changes easier to review and revert independently. The existing helpers are proportionate; this is a recommendation about PR focus.
Targeted attribution, cost/budget, worker, and usage checks passed, and CI is green. Local validation had one unrelated raw-byte directory fixture blocked by the sandbox; the broader Python run was stopped before completion. Approving with the scope recommendation above.
The reason will be displayed to describe this comment to others. Learn more.
Owner-level merge-safety review. Thanks for preserving caller controls while consolidating attribution and usage handling. I found no material unresolved issues. Metadata/configuration propagation, missing-versus-zero usage, measured-budget enforcement and retry-thread retention are consistent across the reviewed paths.
Validation included 153 Action tests, 120 fresh/resumed worker launches, eight real SQLite/rollout cases with base comparisons, native request probes, schema migrations, builds, types, formatting and portable checks. The collector probe reproduced five incorrect complete-usage classifications on the base while preserving measured controls.
Local full SDK/Python runs encountered ownership, virtual Git and fixture-environment constraints; I do not count those suites as passing. Downloaded PR validation reports passed all 879 affected SDK tests (seven skips), 168 usage/migration tests (three skips), and the MCP executor/store/migration checks. No live model or publication request was made.
Please review the updated head dc54791af974a12ec51d7379a4b90a2a76e15f46 after integrating main and the shared test-fixture repairs. The conflict resolution preserves main's native configuration pass-through and usage roles, time windows, rate-limit snapshots, and turn-coverage warnings. Missing worker measurements remain unknown; the direct coordinator's control-only session remains a known zero. The worker-thread registry is now migration 51, leaving every existing main migration unchanged.
Python usage/migration coverage passed after targeted assertion updates, including retention of rate-limit snapshots from unmeasured workers. The Action bundle was rebuilt and checked; current local validation and environment limitations are recorded in the PR description. Hosted CI must pass before merge.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Cross-process telemetry controls, budget accounting, and the persistent schema migration need successful hosted CI and final human review given the reported verification gaps.
No open issues 🎉
What changed in this PR
Centralizes request attribution across the CLI, SDK, GitHub Action, and Deep Scan workers while preserving telemetry controls and improving usage accounting.
Changes:
Shares attribution while retaining caller metadata, profile settings, and OpenTelemetry configuration.
Distinguishes missing usage from measured zero and enforces budgets against measured subtotals.
Preserves worker retry history through migration 51 and expands regression coverage.
The reason will be displayed to describe this comment to others. Learn more.
Thanks for preserving caller telemetry controls while consolidating request attribution and usage accounting. Owner-level merge-safety review: no material unresolved findings. Missing worker measurements remain unknown, measured subtotals still enforce budgets, and migration 51 retains retry-thread history without changing existing migrations.
Validation included 161 Action tests, 120 fresh/resumed discovery/reducer launches, 17 real SQLite/rollout cases, cost and metadata tests, native configuration checks, builds, types, formatting, and portable checks. Current CI reports for a merge with an identical source tree passed the relevant Python and MCP coverage plus two Linux SDK shards. Local runtime suites encountered ownership/Git fixture constraints and are not counted as passing; full cross-platform CI was still running at review time.
Integrated main in 0471da6 after Windows shard 2 reached the old 25-minute timeout. This brings in the merged 40-minute CI budget and Windows credential handling; the telemetry changes are unchanged. Formatting and diff checks pass. Fresh hosted CI is pending.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Cross-process telemetry, budget accounting, and the persistent migration need fresh CI and human validation, alongside the retained attribution correction.
Updated head efc4ec3 addresses the provider-attribution review finding with a two-line implementation/test change. Configured and inherited-context smoke checks, formatting, and diff checks pass. Please review the updated head; fresh CI is pending.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Cross-process configuration and persistent usage changes need final human review and pending cross-platform validation, with a usage-history issue still unresolved.
No open issues 🎉
1 previously missed issue
In code that hasn't changed since last review
Preserve account rate limits when token coverage is unavailable
When an owned rollout has an account-rate-limit update (token_count with info: null) but no token receipt, this return discards the rate_limits collected above. The saved usage then loses observed allowance history, although token coverage and account-rate-limit observations are independent. Keep coverage unavailable, but include any collected accountRateLimits; add a regression case where no thread has measured tokens.
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing the provider attribution. Owner-level merge-safety review: one usage-history issue remains. I independently confirmed the reported allowance-snapshot loss; the inline comment adds the base/head reproduction and the persistence-contract change needed to fix it.
Current-head metadata tests, native configuration checks, builds, types, and formatting pass. The additional real SQLite/rollout probe confirms the regression while its measured-token control passes. Earlier Action, accounting, migration, worker-launch, and portable-check evidence remains applicable to unchanged inputs. Full local suites retain the documented ownership/Git fixture limitations, and current cross-platform CI is still pending.
Allowance snapshots from owned sessions now survive missing token measurements and remain available during pending finalization. This preserves unavailable token coverage and existing costs without fabricating zero totals.
All 127 usage tests, 12 request-metadata tests, and the required portable source checks pass. Please review this follow-up and its parent/worker, delayed-receipt, and persisted-contract coverage.
Integrated main while preserving telemetry forwarding, Daybreak settings, and Windows policy credential protections. The allowance-snapshot fix remains intact and all 127 usage tests pass. MCP worker-launch checks, focused SDK validation, required portable checks, type checks, and Action build/distribution checks completed; the PR body records the local temporary-path limitation and successful focused rerun. Please review this integration before merge.
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing the allowance-history regression. Owner-level merge-safety review at 41983f1: no material unresolved findings. Allowance readings survive unavailable token measurements and pending finalization without inventing token totals; existing costs and sealed artifacts remain intact. The main integration preserves telemetry forwarding, Daybreak settings, and credential protections.
Current-head validation passed 524 focused SDK/CLI tests (three platform skips), MCP worker/database/store checks, 171 Action tests, Action metadata/docs/distribution checks, 12 SQLite lifecycle/validator probes, builds, types, formatting, and portable source checks. The 380 Python usage/database/migration passes (three platform skips) are reused from 9e9db95 after verifying unchanged Linux inputs. Full SDK suites and Windows runtime checks were not rerun.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
kmbroai
left a comment
alandelong-oai
left a comment
Summary
Centralize request attribution across the CLI, SDK, and helpers. GitHub Action runs carry integration and configured-provider attribution, while token totals remain unknown when measurements are missing. Preserve account allowance snapshots independently of token measurements.
Changes
cheaperinferencebuilt-in provider and concurrent request contexts.Testing
After integrating main at
06201edfa54a83fdfc3a9d0e28bfb019ce30b4c0:Risk and rollout
No new CLI flags, Action inputs, collectors, or export destinations. The Action retains
analytics.enabled=false; request attribution remains separate from native analytics and explicitly configured OTel exporters. Provider tags describe wrapper configuration; native managed-provider overrides can select a different backend. Attribution does not establish OpenAI-side reporting for other providers.Migration 51 adds a local worker-thread registry, backfills known associations, and retains them until the scan is deleted. Allowance-only records retain unavailable token coverage and preserve existing legacy cost values.
Public disclosure review