review-migration

name

review-migration

description

Review a migrated or augmented instrumentation-genai package by comparing it against any known external upstream implementations of the same instrumentation (OpenInference, vendor-specific). Handles both greenfield migrations and augment-mode PRs that add coverage to a pre-existing package (checking the added parts, their consistency with existing code, and old-vs-new coexistence). Writes MIGRATION_REPORT.md in the migrated package root.

Review a migrated instrumentation-genai package

Compare the migrated package in instrumentation/<target>/ against every known upstream implementation of the same instrumentation, and write a MIGRATION_REPORT.md in the migrated package root listing only what's missing or wrong.

Inputs and upstreams

User supplies the migrated package name, e.g. opentelemetry-instrumentation-genai-openai. Strip opentelemetry-instrumentation-genai- to get <lib>; that drives the upstream lookup.

Upstreams come from the user, with a default fallback:

• If the user names one or more upstreams (a repo, a directory path, a vendor's own SDK-side instrumentation, a Logfire / Pydantic AI package, …), compare against exactly those — one column per upstream. Use them verbatim; don't second-guess or substitute.

• If the user names none, default to OpenInference: search https://github.com/open-telemetry/donation-openinference under python/instrumentation/ for a package matching <lib> (typically openinference-instrumentation-<lib>, but confirm by listing the directory — the name may differ). Fetch a shallow clone if you don't already have one locally:

  git clone --depth=1 https://github.com/open-telemetry/donation-openinference.git /tmp/openinference
  ls /tmp/openinference/python/instrumentation/ | rg <lib>
  

If there are no upstreams to compare against (no user-named upstream resolves and no OpenInference match), this isn't a migration — bail with a one-line note.

Greenfield vs augment mode

Detect the mode before reviewing — it changes what counts as a problem:

• Greenfield migration — the migration created the package; everything in instrumentation/<target>/ is the migrated package.
• Augment mode — the package already existed and the migration only adds coverage mined from the upstream. The diff modifies a pre-existing package.

Tell them apart from the PR diff / git history / unstaged changes: if the package's src/ and tests predate the migration commits, it's augment mode.

In augment mode the deliverable shifts to: the added coverage's completeness vs the upstream, its consistency with the pre-existing code, and old-vs-new coexistence concerns. Do not flag pre-existing coverage the upstream also has as missing. Per-section adjustments are inline below.

Rules

• Deliverable is instrumentation/<target>/MIGRATION_REPORT.md. Write there, not stdout. The file is gitignored — regenerate freely.
• Report problems, not work. No "✅ matches", no "all tests migrated", no recap of structure. The PR diff already shows what's there; the report is for what's missing or wrong.
• Don't justify empty findings. When a section/table has no problems, emit _none_ (or skip the section, where the rule says so) and stop. Do not list which greps you ran, which env vars you searched for, or why you concluded clean. Clean means clean.
• Tables only when there are ≥1 problem rows. Empty section: _none_.
• Read, don't guess. Every claim from actual code or command output. Don't infer from package names or READMEs. Before listing something as missing, grep for it in the migrated package's src/ and tests/.
• Snapshot of the PR head, not aspirational state.

Report structure

First line:

# Migration review: <target-package>

Mode: greenfield migration | augment existing package

Compared against:
- OpenInference: `openinference-instrumentation-<lib>` (add link)
- <other upstream>: `<path>`, `<link>` (omit the line if no other upstream
  is in scope)

State the mode on the second line. In augment mode, one sentence after it naming what the PR adds (the gaps it closes) orients the reviewer.

Sections render in order. Each is bounded to its problems.

1. Instrumented API surface

Table of every API method any source patches, marking methods the migrated package does not patch. Be exhaustive. Enumerate every endpoint upstream emits a span for, even if attribute extraction is generic. Do not collapse rows.

For each upstream, walk the actual instrumentation code, not docs:

• Method-level patching (the shape of this migration, and of any method-level upstream): read every wrap_function_wrapper(...) call in _instrument(). Each call = one row.
• Transport-level patching (typical of OpenInference, e.g. openai.OpenAI.request / AsyncOpenAI.request): enumerate every cast_to response type the accumulator/dispatch table handles and every other endpoint that flows through the wrapped method (assistants, threads, files, fine-tuning, images, audio, vector_stores, batches, uploads, moderations, …). Spans with only generic attribute extraction still count — list them, and note "generic span only" in the Notes.
• Anything else (vendor SDK hooks, monkey-patched class methods, decorator-based instrumentation): walk the actual entry points the upstream registers and list one row per emitted span site.

One column per upstream that exists, plus This package:

API method	OpenInference	This package	Notes
openai.resources.chat.completions.Completions.create	✅	✅
openai.resources.responses.Responses.create	✅	❌
openai.resources.beta.assistants.Assistants.create	✅ (generic span only)	❌

• ✅ = patched. ❌ = at least one upstream patches it, this migration doesn't. — = not patched (and that's expected — upstream doesn't patch it either).
• Sort ❌ rows to the top.
• For ❌ rows, the Notes cell must name the GenAI semconv operation the method maps to (chat, embeddings, text_completion, invoke_agent, invoke_workflow, execute_tool, realtime, …) — or "no semconv operation defined yet" if the spec hasn't caught up.

Do not render a separate **Gaps:** bullet list below the table — the table itself is the gap list.

Augment mode. Split This package into Existed and Added this PR:

API method	OpenInference	Existed	Added this PR	Notes
…chat.completions.Completions.create	✅	✅	—	already covered
…responses.Responses.create	✅	❌	✅	added by this PR
…batches.Batches.create	✅	❌	❌	still missing — chat

❌ in both columns (upstream patches it, the package still doesn't) is the gap; sort it to the top.

2. Gaps and open issues

Genuine tooling/util gaps — things the migration couldn't do because opentelemetry-util-genai and/or the GenAI semconv doesn't yet support them (missing util-genai factory, attribute not in the registry yet). Reference failing/skipped tests if any.

Do NOT list "OTel semconv doesn't cover X" as a gap when X is just an inference API variant — semconv applies to all inference APIs.

Gap	File / test	Upstream issue	Notes

3. Significant behavioral changes

Material differences vs upstream — equivalences (different code shape, same emitted telemetry) are not listed. Look for: patching strategy (transport-level vs method-level); error recording (exception event vs error.type + status); token counting (cache details folded vs separate); message format (flat OpenInference attributes vs JSON parts array); streaming wrapper shape; attributes set unconditionally vs gated on is_recording() / should_capture_content(); anything emitted by upstream that this migration deliberately drops without a one-line rationale.

Aspect	Upstream	This package	Notes

3b. Consistency and old-vs-new coexistence (augment mode only)

Greenfield migration: skip. Here the risk isn't "is it conformant" but "do the additions fit the package they landed in." Compare the added code against the pre-existing code (not the upstream); flag only real problems:

• Divergent patterns — added wrappers/helpers/fixtures re-implementing something the package already has a convention for.
• Duplicated scaffolding — added tests not reusing the package's tests/test_utils.py helpers or conformance runner.
• Telemetry contradictions — an added method emitting an operation name, attribute shape, span-kind, or content-capture gating that disagrees with what pre-existing methods emit for the analogous case.
• Coexistence hazards — added code that alters pre-existing paths (shared module-level state, _instrument() ordering, a dependency range the old code wasn't tested against), or a pyproject/tox edit affecting existing tests.

Concern	Pre-existing	Added	Notes

_none_ if the additions are consistent and isolated.

4. Test coverage

Three checklists. Render only missing cells; if every checklist is clean, render _Test coverage complete._ and skip the subsections.

Gaps in this section are blockers for the migration PR, not follow-up work — address them before merge. Do not list §4 items as follow-up issues in §5.

Augment mode. The matrix applies to methods added this PR (§1 Added this PR = ✅) — those block merge. A pre-existing method missing a variant is a §5 follow-up, not a §4 blocker.

Also flag here: 🟡 missing-cassette — any scenario or unit test that references a cassette not committed under tests/cassettes/. And: unreferenced cassettes — one-line count of tests/cassettes/*.yaml files no test/scenario opens. Skip if 0.

For each unreferenced cassette, walk git history before naming a cause:

1. Check whether the same cassette is also unreferenced in the upstream it came from (for each unreferenced file, search the upstream's tests/ for any reference). If yes, it's an inherited upstream orphan — say so plainly; the migration is not responsible.
2. If the cassette IS referenced upstream but not in the migrated package, the migration dropped a test. That is not a §4b cosmetic note — it's a §4a missing variant (or a missing scenario, depending on what the test covered). Add it to §4a's table with the upstream test name in Notes, and do not rationalize ("tests were renamed/removed during the migration" without naming the commit is speculation, not analysis).

Never write "appear to be" / "likely" / "safe to delete" without evidence. Either you walked the history and know, or you didn't — in which case say "history not checked" so the reviewer does it.

4a. Unit-test matrix per wrapped method

For each method in §1 row where This package = ✅:

Variant	Required when
sync × happy	always
sync × error	always — verify the original exception is re-raised unmodified, error.type is recorded, span status is ERROR
async × happy	the lib exposes an async counterpart
async × error	the lib exposes an async counterpart
streaming × happy	the method accepts stream=True or returns a stream wrapper
streaming × error	flag if streaming is supported but no error path is exercised at all or error is not recorded on telemetry
async streaming × happy	the method accepts stream=True or returns an async stream wrapper
async streaming × error	flag if streaming is supported but no error path is exercised at all or error is not recorded on telemetry

Identify variants by reading the migrated package's src/ (is_streaming(kwargs), async def, Stream / AsyncStream wrappers).

Wrapped method	Missing variants	Notes

4b. Conformance scenarios

For each distinct GenAI semconv operation the migrated package emits (chat, embeddings, execute_tool, invoke_agent, invoke_workflow, create_agent, …) there should be at least one happy-path scenario file under tests/conformance/<op>.py driven by run_conformance(...). More scenarios per operation are fine but never required.

Operation	Scenario file	Status

Mention if conformance scenario is skipped, there are expected_violations, or uv run tox -e py312-test-instrumentation-genai-<lib> fails.

4c. Docstring / README coverage

Asset	Required content	Status
README.rst	install snippet, usage snippet importing from opentelemetry.instrumentation.genai.<lib>, pointer to OTEL_INSTRUMENTATION_GENAI_CAPTURE_MESSAGE_CONTENT and the util-genai README, pointer to tests/conformance/ (no examples/)
src/opentelemetry/instrumentation/genai/<lib>/__init__.py module docstring	usage example using the new module path; configuration section listing the env vars users actually need

Render only rows that are missing/wrong/stale (e.g. README still imports from openinference.instrumentation.<lib>, install command points at the openinference-instrumentation-<lib> package name, links to an OpenInference URL).

Refactor misses. One bullet only if unit tests re-implement generic semconv shape checks inline instead of factoring them into the migrated package's tests/test_utils.py helpers (e.g. assert_all_attributes, assert_completion_attributes, assert_messages_attribute). There is no shared opentelemetry.test_util_genai.assertions module — assert directly on span.attributes[GenAIAttributes.GEN_AI_…] using the semconv constants.

5. Follow-up work

What goes in this PR vs. a follow-up:

• In this PR: §4 test coverage gaps. These block merge. In augment mode, scoped to the methods added this PR, plus any §3b consistency problem in the added code (it ships here).
• Follow-up: §1 ❌ rows (new instrumented methods), §2 util-genai / semconv gaps, §3 behavioral parity items, and — augment mode — pre-existing coverage gaps from §4 — each a separate PR, one logical change apiece.

Suggest an issue title per follow-up item, grouped by type (API surface / util-genai gaps / behavioral parity); name the upstream that covers it and the semconv operation where relevant. Do not file the issues — listing them is the deliverable; filing AI-generated issues is against the contributor policy, so the human author decides which to open. If there are none, render _No follow-up issues recommended._

See also

• .github/skills/migrate-from-openinference/SKILL.md — the migration skill; it runs this review at its final step to produce MIGRATION_REPORT.md.
• .github/skills/write-conformance-tests/SKILL.md — authoring the conformance scenarios this report checks in §4b.
• .github/instructions/instrumentation.instructions.md — the copilot PR-review rules for instrumentation/**; generic instrumentation violations are flagged there and not repeated in this report.