Skip to content

Conversation

yanomaly
Copy link
Contributor

@yanomaly yanomaly commented Jul 31, 2025

Hi, there! I'm implementing custom instrumentation for Writer's models. Here are our docs and official web-site.

  • I have added tests that cover my changes.
  • If adding a new instrumentation or changing an existing one, I've added screenshots from some observability platform showing the change.
  • PR name follows conventional commits format: feat(instrumentation): ... or fix(instrumentation): ....
  • I have updated the documentation accordingly.

Summary by CodeRabbit

  • New Features
    • Added OpenTelemetry instrumentation for Writer: tracing, event emission, streaming and tool-call telemetry, and token/latency metrics.
  • Documentation
    • New package README and main docs list Writer as an instrumented provider; includes install, usage, and privacy controls.
  • Tests
    • Added extensive fixtures covering streaming, non-streaming, multi-choice, and tool-call scenarios.
  • Chores
    • Added packaging, project config, linting config, Python version pin (3.12.5), and package version 0.46.2.

@CLAassistant
Copy link

CLAassistant commented Jul 31, 2025

CLA assistant check
All committers have signed the CLA.

Copy link

coderabbitai bot commented Jul 31, 2025

Walkthrough

Adds a new opentelemetry-instrumentation-writer package: instrumentation code (sync/async wrappers, streaming accumulation, metrics), span/event utilities and models, configuration and packaging files, many VCR test cassettes, and a README entry listing Writer as instrumented.

Changes

Cohort / File(s) Summary
Docs
README.md, packages/opentelemetry-instrumentation-writer/README.md
Adds Writer to the top-level instrumented list and introduces package README with install, usage, example, and content-logging controls.
Instrumentation Core
packages/opentelemetry-instrumentation-writer/opentelemetry/instrumentation/writer/__init__.py
New WriterInstrumentor implementing sync/async wrappers, CLIENT span creation, streaming accumulation, error handling, metrics histograms, suppression checks, and registration/unregistration.
Config & Version
packages/opentelemetry-instrumentation-writer/opentelemetry/instrumentation/writer/config.py, .../version.py
Adds Config (exception_logger, use_legacy_attributes) and __version__ = "0.46.2".
Span / Event Utilities & Models
.../writer/span_utils.py, .../writer/event_emitter.py, .../writer/event_models.py, .../writer/utils.py
New helper modules: span attribute setters, response/model normalization, event emission, typed event models, safe helpers, metrics/attribute builders, and streaming initialization utilities.
Packaging & Tooling
packages/opentelemetry-instrumentation-writer/pyproject.toml, packages/opentelemetry-instrumentation-writer/project.json, packages/opentelemetry-instrumentation-writer/poetry.toml, packages/opentelemetry-instrumentation-writer/.flake8, packages/opentelemetry-instrumentation-writer/.python-version, .cz.toml
Adds Poetry packaging and OTel plugin entry, Nx project config/targets, Flake8 config, pinned Python version file, and commitizen version tracking entries.
Tests / Cassettes
packages/opentelemetry-instrumentation-writer/tests/__init__.py, packages/opentelemetry-instrumentation-writer/tests/cassettes/test_chat/*
Adds tests package and many VCR cassettes covering sync/async, streaming/non-streaming, multi-choice, tool-calls, and event-emission scenarios.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor App
  participant Instr as WriterInstrumentor
  participant SDK as Writer SDK
  participant OTel as OpenTelemetry

  App->>Instr: instrument()
  Note right of Instr #E8F8FF: wrap Writer SDK methods (sync & async)
  App->>SDK: chat/completions(...)
  Instr->>OTel: start CLIENT span (attrs: gen_ai.system="writer", request_type)
  Instr->>OTel: set input/model attrs or emit events
  SDK-->>Instr: response (sync) or stream iterator (async)
  alt Non-streaming
    Instr->>OTel: set response attrs / emit events
    Instr->>OTel: record duration & token metrics
    Instr->>OTel: end span (OK)
    Instr-->>App: return response
  else Streaming
    loop per chunk
      SDK-->>Instr: chunk
      Instr->>Instr: accumulate chunk into final structure
      Instr-->>App: yield chunk
      opt first token
        Instr->>OTel: record time-to-first-token metric
      end
    end
    Instr->>OTel: record total generation time & metrics
    Instr->>OTel: set final response attrs / emit events
    Instr->>OTel: end span (OK)
    Instr-->>App: stream complete
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Suggested reviewers

  • doronkopit5

Poem

A twitch of whiskers, I patch the stream,
Spans hop like carrots in a tracing dream.
I gather chunks and count each token,
I emit small events, politely unbroken.
Hooray — Writer traced! A little rabbit cheer 🐇✨


📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between a3e8821 and f9d11be.

⛔ Files ignored due to path filters (1)
  • packages/opentelemetry-instrumentation-writer/poetry.lock is excluded by !**/*.lock
📒 Files selected for processing (3)
  • .cz.toml (1 hunks)
  • packages/opentelemetry-instrumentation-writer/opentelemetry/instrumentation/writer/version.py (1 hunks)
  • packages/opentelemetry-instrumentation-writer/pyproject.toml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • packages/opentelemetry-instrumentation-writer/opentelemetry/instrumentation/writer/version.py
  • packages/opentelemetry-instrumentation-writer/pyproject.toml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: traceloop/openllmetry#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-17T15:06:48.109Z
Learning: Instrumentation packages must leverage the semantic conventions package and emit OTel-compliant spans
🔇 Additional comments (2)
.cz.toml (2)

65-66: Writer package added to version bump tracking — LGTM.

Both pyproject.toml:^version and version.py are correctly listed and consistent with the existing pattern.


65-66: Version sync verified – .cz.toml, pyproject.toml, and version.py all use 0.46.2.

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore or @coderabbit ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

yanomaly and others added 25 commits August 1, 2025 17:23
- Implement non-streaming response processor
- Add message events emitter
- Create event emitter system with choice and message events
- Implement model_as_dict utility
- Add span attribute setters
- Support both chat completion and text generation response formats
Completed async and sync streaming response processor methods in __init__ file
Finished remaining TODOs for response type check and tool calls setter
Adding WRITER to the list of providers
…iter' into opentelemetry-instrumentation-writer
@yanomaly yanomaly marked this pull request as ready for review August 27, 2025 11:12
Copy link
Contributor

ellipsis-dev bot commented Aug 27, 2025

⚠️ This PR is too big for Ellipsis, but support for larger PRs is coming soon. If you want us to prioritize this feature, let us know at [email protected]


Generated with ❤️ by ellipsis.dev

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 18

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
packages/opentelemetry-instrumentation-writer/tests/cassettes/test_chat/test_writer_async_chat_multiple_tool_call_requests_with_events_with_content.yaml (1)

1-40: Scrub request/response bodies in Writer cassettes
Cassettes under packages/opentelemetry-instrumentation-writer/tests/cassettes include hard-coded API keys and credentials in request bodies (e.g. apikey=test_api_key, service_id, access_key_id, secret_access_key). The existing VCR configurations only filter headers—bodies remain unredacted. We need to redact these fields in both requests and responses.

• packages/opentelemetry-instrumentation-writer/tests/cassettes/**/*.yaml
– Detected fields: api_key, service_id, access_key_id, secret_access_key (lines 3, 148 in the sample cassette)
• tests/conftest.py (all VCR fixtures)
– Extend each vcr_config() to include before_record_request and before_record_response hooks (or filter_post_data) that strip or replace those sensitive keys in JSON bodies

Please add a shared scrub utility (e.g. _scrub_body(request_or_response, [...])) and apply it across all Writer test suites to prevent any secrets from being committed in VCR cassettes.

packages/opentelemetry-instrumentation-writer/tests/cassettes/test_chat/test_writer_chat_multiple_choices_with_events_with_no_content.yaml (1)

1-35: Ensure fixture name matches its content

I’ve verified that there are no exposed API keys or tokens in the packages/opentelemetry-instrumentation-writer/tests/cassettes directory (secret scan returned no matches).

However, the cassette

  • packages/opentelemetry-instrumentation-writer/tests/cassettes/test_chat/test_writer_chat_multiple_choices_with_events_with_no_content.yaml
    still contains non-null message.content fields despite its “_with_no_content” name.

Please address this discrepancy by either:

  • Renaming the file to reflect that it includes content (e.g. …with_content.yaml), or
  • Regenerating the fixture so that all message.content values are null.
packages/opentelemetry-instrumentation-writer/tests/cassettes/test_chat/test_writer_streaming_chat_multiple_choices_with_events_with_no_content.yaml (1)

447-456: Refine CI cassette leak check to avoid false positives

The current rg command is matching every occurrence of "token" in VCR cassettes (e.g. prompt_tokens, completion_tokens), resulting in noisy alerts. We need to narrow the pattern to only catch actual secrets or authorization headers.

Please update the script as follows:

• Only match sensitive header names or common secret keywords (e.g. Authorization, api_key, x-api-key, bearer , secret)
• Exclude telemetry metadata fields ending in _tokens or _token_details

Example revised snippet:

#!/bin/bash
# Fail CI if any cassette contains suspicious tokens/headers (ignoring "_tokens" metrics)
set -euo pipefail

# Find candidate lines
matches=$(
  rg -n \
    -g 'packages/**/cassettes/**/*.ya*ml' \
    -e 'Authorization|authorization|api[-_ ]?key|x-api-key|bearer[[:space:]]|secret' \
    || true
)

# Filter out common metrics fields like prompt_tokens, completion_tokens, token_details
filtered=$(echo "$matches" | grep -Ev '"(prompt|completion)(_tokens?|_token_details)":')

if [[ -n "$filtered" ]]; then
  echo "Potential secret/PII detected in cassettes:"
  echo "$filtered"
  exit 1
fi

echo "Cassette scan passed."

• The grep -Ev '"(prompt|completion)(_tokens?|_token_details)":' step removes lines that only report token counts or details.
• You may adjust the exclusion regex if other legitimate “token” fields exist.

After applying this change, please run the script again to confirm no false positives.

♻️ Duplicate comments (4)
packages/opentelemetry-instrumentation-writer/tests/cassettes/test_chat/test_writer_streaming_chat_tool_calls_with_events_with_no_content.yaml (1)

1-186: Apply cassette sanitization as noted in sibling file.

Same recommendation to scrub id/chatcmpl-tool-*/created/system_fingerprint for stability.

packages/opentelemetry-instrumentation-writer/tests/cassettes/test_chat/test_writer_streaming_chat_multiple_tool_call_requests_with_events_with_content.yaml (1)

1-121: Sanitize volatile identifiers and timestamps.

Mirror the before_record scrubbing to mask ids, tool ids, created, and system_fingerprint.

packages/opentelemetry-instrumentation-writer/tests/cassettes/test_chat/test_writer_async_streaming_chat_tool_calls_with_events_with_content.yaml (1)

1-186: Apply identifier/timestamp scrubbing as elsewhere.

packages/opentelemetry-instrumentation-writer/tests/cassettes/test_chat/test_writer_async_streaming_chat_multiple_tool_call_requests_legacy.yaml (1)

1-121: Scrub volatile IDs and timestamps for deterministic replays.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

♻️ Duplicate comments (4)
packages/opentelemetry-instrumentation-writer/opentelemetry/instrumentation/writer/__init__.py (4)

177-183: TTFT/TTG capture moved to first chunk — good fix.

Setting first_token_time on the first received chunk corrects TTFT/TTG. Thanks for addressing the earlier feedback.

Also applies to: 192-200


226-232: Async TTFT/TTG capture is correct too.

Same positive note for the async path.

Also applies to: 241-249


337-339: Unified LLM system casing (“writer”).

Matches metrics attributes and avoids cardinality skew.


438-440: Async span uses “writer” casing consistently.

Good.

🧹 Nitpick comments (4)
packages/opentelemetry-instrumentation-writer/opentelemetry/instrumentation/writer/__init__.py (4)

72-74: Nit: merge isinstance checks.

Use a tuple for a single isinstance call (matches Ruff SIM101).

-def is_streaming_response(response):
-    return isinstance(response, Stream) or isinstance(response, AsyncStream)
+def is_streaming_response(response):
+    return isinstance(response, (Stream, AsyncStream))

92-103: Harden index handling in streamed deltas (avoid IndexError/TypeError).

choice_index/tool_index may be missing/None in some deltas. Default to 0 and guard math with int().

-            choice_index = chunk.choices[0].index
+            choice_index = chunk.choices[0].index or 0
@@
-                    tool_index = chunk.choices[0].delta.tool_calls[0].index
+                    tool_index = chunk.choices[0].delta.tool_calls[0].index or 0
@@
-                        enhance_list_size(
+                        enhance_list_size(
                             accumulated_response.choices[
                                 choice_index
                             ].message.tool_calls,
-                            tool_index + 1,
+                            int(tool_index) + 1,
                         )

If Writer’s SDK guarantees non-null indexes, feel free to ignore; otherwise this avoids hard crashes mid-stream.

Also applies to: 112-150


545-547: Optional: accept common truthy/falsey values for metrics env.

Support “1/0, yes/no, on/off” for robustness.

-def is_metrics_collection_enabled() -> bool:
-    return (os.getenv("TRACELOOP_METRICS_ENABLED") or "true").lower() == "true"
+def is_metrics_collection_enabled() -> bool:
+    val = (os.getenv("TRACELOOP_METRICS_ENABLED") or "true").strip().lower()
+    return val in {"true", "1", "yes", "on"}

594-609: Style: use contextlib.suppress for import-optional wrappers (Ruff SIM105).

Cleaner than try/except/pass.

+from contextlib import suppress
@@
-        for wrapped_method in WRAPPED_METHODS:
+        for wrapped_method in WRAPPED_METHODS:
@@
-            try:
-                wrap_function_wrapper(
+            with suppress(ModuleNotFoundError):
+                wrap_function_wrapper(
                     wrap_package,
                     f"{wrap_object}.{wrap_method}",
                     _wrap(
                         tracer,
                         token_histogram,
                         duration_histogram,
                         streaming_time_to_first_token,
                         streaming_time_to_generate,
                         event_logger,
                         wrapped_method,
                     ),
                 )
-            except ModuleNotFoundError:
-                pass
@@
-        for wrapped_method in WRAPPED_AMETHODS:
+        for wrapped_method in WRAPPED_AMETHODS:
@@
-            try:
-                wrap_function_wrapper(
+            with suppress(ModuleNotFoundError):
+                wrap_function_wrapper(
                     wrap_package,
                     f"{wrap_object}.{wrap_method}",
                     _awrap(
                         tracer,
                         token_histogram,
                         duration_histogram,
                         streaming_time_to_first_token,
                         streaming_time_to_generate,
                         event_logger,
                         wrapped_method,
                     ),
                 )
-            except ModuleNotFoundError:
-                pass

Also applies to: 615-630

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between e44199d and a80c2bf.

📒 Files selected for processing (2)
  • packages/opentelemetry-instrumentation-writer/README.md (1 hunks)
  • packages/opentelemetry-instrumentation-writer/opentelemetry/instrumentation/writer/__init__.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/opentelemetry-instrumentation-writer/README.md
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.py: Store API keys only in environment variables/secure vaults; never hardcode secrets in code
Use Flake8 for code linting and adhere to its rules

Files:

  • packages/opentelemetry-instrumentation-writer/opentelemetry/instrumentation/writer/__init__.py
🧬 Code graph analysis (1)
packages/opentelemetry-instrumentation-writer/opentelemetry/instrumentation/writer/__init__.py (5)
packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/__init__.py (2)
  • Meters (36-61)
  • SpanAttributes (64-261)
packages/opentelemetry-instrumentation-writer/opentelemetry/instrumentation/writer/config.py (1)
  • Config (1-3)
packages/opentelemetry-instrumentation-writer/opentelemetry/instrumentation/writer/event_emitter.py (2)
  • emit_choice_events (57-87)
  • emit_message_events (35-53)
packages/opentelemetry-instrumentation-writer/opentelemetry/instrumentation/writer/span_utils.py (4)
  • set_input_attributes (13-56)
  • set_model_input_attributes (60-75)
  • set_model_response_attributes (79-125)
  • set_response_attributes (129-137)
packages/opentelemetry-instrumentation-writer/opentelemetry/instrumentation/writer/utils.py (9)
  • enhance_list_size (148-156)
  • error_metrics_attributes (65-70)
  • initialize_accumulated_response (113-127)
  • initialize_choice (130-139)
  • initialize_tool_call (142-145)
  • request_type_by_method (73-79)
  • response_attributes (83-90)
  • should_emit_events (93-99)
  • wrapper (49-59)
🪛 Ruff (0.12.2)
packages/opentelemetry-instrumentation-writer/opentelemetry/instrumentation/writer/__init__.py

73-73: Multiple isinstance calls for response, merge into a single call

Merge isinstance calls for response

(SIM101)


594-609: Use contextlib.suppress(ModuleNotFoundError) instead of try-except-pass

Replace with contextlib.suppress(ModuleNotFoundError)

(SIM105)


615-630: Use contextlib.suppress(ModuleNotFoundError) instead of try-except-pass

Replace with contextlib.suppress(ModuleNotFoundError)

(SIM105)

🔇 Additional comments (2)
packages/opentelemetry-instrumentation-writer/opentelemetry/instrumentation/writer/__init__.py (2)

302-308: Response attribute emission path looks good.

Token usage gated inside span_utils; events vs legacy attributes respected.


262-268: Input attributes/events handling is correct.

Clean separation between legacy attributes and events.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
packages/opentelemetry-instrumentation-writer/opentelemetry/instrumentation/writer/__init__.py (1)

40-41: Update instrumentation dependency to correct PyPI package name.

Based on the imports using from writerai.*, the correct package name should be writerai, not writer-sdk. This mismatch would prevent auto-instrumentation from working correctly.

Apply this diff to fix the package dependency:

-_instruments = ("writer-sdk >= 2.2.1, < 3",)
+_instruments = ("writerai >= 4.0.1, < 5",)
🧹 Nitpick comments (6)
packages/opentelemetry-instrumentation-writer/opentelemetry/instrumentation/writer/__init__.py (6)

72-74: Simplify isinstance check by merging conditions.

The static analyzer correctly suggests merging the multiple isinstance calls for better readability.

-def is_streaming_response(response):
-    return isinstance(response, Stream) or isinstance(response, AsyncStream)
+def is_streaming_response(response):
+    return isinstance(response, (Stream, AsyncStream))

178-222: Missing exception recording on span in streaming processor.

The exception handling properly sets error status but doesn't record the exception details on the span, which reduces observability.

     except Exception as ex:
         error = ex
         if span.is_recording():
             span.set_status(Status(StatusCode.ERROR))
+            span.record_exception(ex)
         raise

238-281: Missing exception recording on span in async streaming processor.

Same issue as the sync version - exception details should be recorded on the span for better observability.

     except Exception as ex:
         error = ex
         if span.is_recording():
             span.set_status(Status(StatusCode.ERROR))
+            span.record_exception(ex)
         raise

369-380: Add exception recording to sync wrapper error handling.

While the error handling correctly sets span status and ends the span, it should also record the exception for better observability.

     except Exception as ex:  # pylint: disable=broad-except
         end_time = time.time()

         if duration_histogram:
             duration = end_time - start_time
             duration_histogram.record(duration, attributes=error_metrics_attributes(ex))

         if span.is_recording():
             span.set_status(Status(StatusCode.ERROR))
+            span.record_exception(ex)

         span.end()
         raise

470-481: Add exception recording to async wrapper error handling.

Mirror the improvement needed in the sync wrapper for consistent exception handling.

     except Exception as ex:  # pylint: disable=broad-except
         end_time = time.time()

         if duration_histogram:
             duration = end_time - start_time
             duration_histogram.record(duration, attributes=error_metrics_attributes(ex))

         if span.is_recording():
             span.set_status(Status(StatusCode.ERROR))
+            span.record_exception(ex)

         span.end()
         raise

616-631: Consider using contextlib.suppress for cleaner exception handling.

The static analyzer suggests using contextlib.suppress for the ModuleNotFoundError handling, which would be cleaner.

Add the import at the top:

+import contextlib
 import logging

Then apply these changes:

-            try:
-                wrap_function_wrapper(
+            with contextlib.suppress(ModuleNotFoundError):
+                wrap_function_wrapper(
                     wrap_package,
                     f"{wrap_object}.{wrap_method}",
                     _wrap(
                         tracer,
                         token_histogram,
                         duration_histogram,
                         streaming_time_to_first_token,
                         streaming_time_to_generate,
                         event_logger,
                         wrapped_method,
                     ),
                 )
-            except ModuleNotFoundError:
-                pass

Apply the same pattern to the async methods block.

Also applies to: 637-652

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between a80c2bf and 1d47a4b.

📒 Files selected for processing (1)
  • packages/opentelemetry-instrumentation-writer/opentelemetry/instrumentation/writer/__init__.py (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.py: Store API keys only in environment variables/secure vaults; never hardcode secrets in code
Use Flake8 for code linting and adhere to its rules

Files:

  • packages/opentelemetry-instrumentation-writer/opentelemetry/instrumentation/writer/__init__.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: traceloop/openllmetry#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-17T15:06:48.109Z
Learning: Instrumentation packages must leverage the semantic conventions package and emit OTel-compliant spans
🧬 Code graph analysis (1)
packages/opentelemetry-instrumentation-writer/opentelemetry/instrumentation/writer/__init__.py (5)
packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/__init__.py (2)
  • Meters (36-61)
  • SpanAttributes (64-261)
packages/opentelemetry-instrumentation-writer/opentelemetry/instrumentation/writer/config.py (1)
  • Config (1-3)
packages/opentelemetry-instrumentation-writer/opentelemetry/instrumentation/writer/event_emitter.py (2)
  • emit_choice_events (57-87)
  • emit_message_events (35-53)
packages/opentelemetry-instrumentation-writer/opentelemetry/instrumentation/writer/span_utils.py (4)
  • set_input_attributes (13-56)
  • set_model_input_attributes (60-75)
  • set_model_response_attributes (79-125)
  • set_response_attributes (129-137)
packages/opentelemetry-instrumentation-writer/opentelemetry/instrumentation/writer/utils.py (9)
  • enhance_list_size (148-156)
  • error_metrics_attributes (65-70)
  • initialize_accumulated_response (113-127)
  • initialize_choice (130-139)
  • initialize_tool_call (142-145)
  • request_type_by_method (73-79)
  • response_attributes (83-90)
  • should_emit_events (93-99)
  • wrapper (49-59)
🪛 Ruff (0.12.2)
packages/opentelemetry-instrumentation-writer/opentelemetry/instrumentation/writer/__init__.py

73-73: Multiple isinstance calls for response, merge into a single call

Merge isinstance calls for response

(SIM101)


616-631: Use contextlib.suppress(ModuleNotFoundError) instead of try-except-pass

Replace with contextlib.suppress(ModuleNotFoundError)

(SIM105)


637-652: Use contextlib.suppress(ModuleNotFoundError) instead of try-except-pass

Replace with contextlib.suppress(ModuleNotFoundError)

(SIM105)

🔇 Additional comments (3)
packages/opentelemetry-instrumentation-writer/opentelemetry/instrumentation/writer/__init__.py (3)

359-361: Use consistent casing for LLM_SYSTEM attribute.

The span attribute uses "writer" (lowercase) which should match the metrics convention consistently throughout the codebase.

The lowercase "writer" value aligns with the metrics attributes from response_attributes() in utils.py.


571-578: LGTM! Clean instrumentation setup.

The instrumentation class properly initializes configuration and follows OpenTelemetry patterns correctly.


589-602: Good conditional metrics initialization.

The metrics are properly conditionally created based on the environment configuration, with clean fallback to None values when disabled.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (7)
packages/opentelemetry-instrumentation-writer/opentelemetry/instrumentation/writer/__init__.py (7)

355-362: LLM system attribute casing is now consistent (“writer”). LGTM.

Also applies to: 456-463


368-381: Record exceptions on the span in the sync wrapper.

You set ERROR but don’t record the exception.

Apply:

     except Exception as ex:  # pylint: disable=broad-except
         end_time = time.time()
@@
         if span.is_recording():
-            span.set_status(Status(StatusCode.ERROR))
+            span.set_status(Status(StatusCode.ERROR))
+            span.record_exception(ex)
 
         span.end()
         raise

395-405: Streaming setup failure: include traceback in logs and record exception on span.

Apply:

-        except Exception as ex:
-            logger.warning(
-                "Failed to process streaming response for writer span, error: %s",
-                str(ex),
-            )
-            if span.is_recording():
-                span.set_status(Status(StatusCode.ERROR))
-
-            span.end()
-            raise
+        except Exception as ex:
+            logger.warning(
+                "Failed to process streaming response for writer span, error: %s",
+                str(ex),
+                exc_info=True,
+            )
+            if span.is_recording():
+                span.set_status(Status(StatusCode.ERROR))
+                span.record_exception(ex)
+            span.end()
+            raise

496-506: Async streaming setup failure: mirror logging and exception recording.

Apply:

-        except Exception as ex:
-            logger.warning(
-                "Failed to process streaming response for writer span, error: %s",
-                str(ex),
-            )
-            if span.is_recording():
-                span.set_status(Status(StatusCode.ERROR))
-
-            span.end()
-            raise
+        except Exception as ex:
+            logger.warning(
+                "Failed to process streaming response for writer span, error: %s",
+                str(ex),
+                exc_info=True,
+            )
+            if span.is_recording():
+                span.set_status(Status(StatusCode.ERROR))
+                span.record_exception(ex)
+            span.end()
+            raise

180-195: Stream metrics correctness: update last_token_time per chunk and record exceptions.

Without updating last_token_time inside the loop, TTG is wrong on early-terminated/erroring streams (can even go negative). Also record the exception on the span.

Apply:

-    try:
-        for chunk in response:
-            if first_token_time is None:
-                first_token_time = time.time()
-
-            _update_accumulated_response(accumulated_response, chunk)
-
-            yield chunk
-
-        last_token_time = time.time()
+    try:
+        for chunk in response:
+            if first_token_time is None:
+                first_token_time = time.time()
+            _update_accumulated_response(accumulated_response, chunk)
+            yield chunk
+            last_token_time = time.time()
     except Exception as ex:
         error = ex
         if span.is_recording():
-            span.set_status(Status(StatusCode.ERROR))
+            span.set_status(Status(StatusCode.ERROR))
+            span.record_exception(ex)
         raise

240-255: Async stream metrics: same fix as sync (update last_token_time and record exceptions).

Apply:

-    try:
-        async for chunk in response:
-            if first_token_time is None:
-                first_token_time = time.time()
-
-            _update_accumulated_response(accumulated_response, chunk)
-
-            yield chunk
-
-        last_token_time = time.time()
+    try:
+        async for chunk in response:
+            if first_token_time is None:
+                first_token_time = time.time()
+            _update_accumulated_response(accumulated_response, chunk)
+            yield chunk
+            last_token_time = time.time()
     except Exception as ex:
         error = ex
         if span.is_recording():
-            span.set_status(Status(StatusCode.ERROR))
+            span.set_status(Status(StatusCode.ERROR))
+            span.record_exception(ex)
         raise

40-41: Fix instrumentation dependency: switch to the writerai package (4.x)

Verified via PyPI that the canonical package names and versions are:

  • writerai → latest 4.0.1
  • writer-sdk → latest 2.3.1

The auto-instrumentation tuple must reference writerai >= 4, < 5, not the old writer-sdk range.

Affected file:

  • packages/opentelemetry-instrumentation-writer/opentelemetry/instrumentation/writer/init.py

Please apply:

-_instruments = ("writer-sdk >= 2.2.1, < 3",)
+_instruments = ("writerai >= 4, < 5",)
🧹 Nitpick comments (2)
packages/opentelemetry-instrumentation-writer/opentelemetry/instrumentation/writer/__init__.py (2)

611-632: Use contextlib.suppress for import-optional wrappers (Ruff SIM105).

Apply:

+import contextlib
@@
-        for wrapped_method in WRAPPED_METHODS:
+        for wrapped_method in WRAPPED_METHODS:
             wrap_package = wrapped_method.get("package")
             wrap_object = wrapped_method.get("object")
             wrap_method = wrapped_method.get("method")
-
-            try:
-                wrap_function_wrapper(
+            with contextlib.suppress(ModuleNotFoundError):
+                wrap_function_wrapper(
                     wrap_package,
                     f"{wrap_object}.{wrap_method}",
                     _wrap(
                         tracer,
                         token_histogram,
                         duration_histogram,
                         streaming_time_to_first_token,
                         streaming_time_to_generate,
                         event_logger,
                         wrapped_method,
                     ),
                 )
-            except ModuleNotFoundError:
-                pass
@@
-        for wrapped_method in WRAPPED_AMETHODS:
+        for wrapped_method in WRAPPED_AMETHODS:
             wrap_package = wrapped_method.get("package")
             wrap_object = wrapped_method.get("object")
             wrap_method = wrapped_method.get("method")
-            try:
-                wrap_function_wrapper(
+            with contextlib.suppress(ModuleNotFoundError):
+                wrap_function_wrapper(
                     wrap_package,
                     f"{wrap_object}.{wrap_method}",
                     _awrap(
                         tracer,
                         token_histogram,
                         duration_histogram,
                         streaming_time_to_first_token,
                         streaming_time_to_generate,
                         event_logger,
                         wrapped_method,
                     ),
                 )
-            except ModuleNotFoundError:
-                pass

Also applies to: 633-652


91-149: Streaming accumulator only processes the first choice/tool_call; iterate over all.

Writer can stream multiple choices and multiple tool_calls per chunk; handling only index 0 can drop data.

Apply (illustrative):

-        if chunk.choices:
-            choice_index = chunk.choices[0].index or 0
+        if chunk.choices:
+            for choice in chunk.choices:
+                choice_index = choice.index or 0
@@
-            if chunk.choices[0].delta:
-                if content := chunk.choices[0].delta.content:
+            if choice.delta:
+                if content := choice.delta.content:
                     accumulated_response.choices[
                         choice_index
                     ].message.content += content
-                if role := chunk.choices[0].delta.role:
+                if role := choice.delta.role:
                     accumulated_response.choices[choice_index].message.role = role
-                if chunk.choices[0].delta.tool_calls:
-                    tool_index = chunk.choices[0].delta.tool_calls[0].index or 0
+                for tool_call in (choice.delta.tool_calls or []):
+                    tool_index = tool_call.index or 0
@@
-                    if name := chunk.choices[0].delta.tool_calls[0].function.name:
+                    if name := tool_call.function.name:
                         accumulated_response.choices[choice_index].message.tool_calls[
                             tool_index
                         ].function.name += name
-                    if (
-                        arguments := chunk.choices[0]
-                        .delta.tool_calls[0]
-                        .function.arguments
-                    ):
+                    if (arguments := tool_call.function.arguments):
                         accumulated_response.choices[choice_index].message.tool_calls[
                             tool_index
                         ].function.arguments += arguments
-                    if tool_id := chunk.choices[0].delta.tool_calls[0].id:
+                    if tool_id := tool_call.id:
                         accumulated_response.choices[choice_index].message.tool_calls[
                             tool_index
                         ].id = tool_id
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 1d47a4b and 68737b3.

📒 Files selected for processing (1)
  • packages/opentelemetry-instrumentation-writer/opentelemetry/instrumentation/writer/__init__.py (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.py: Store API keys only in environment variables/secure vaults; never hardcode secrets in code
Use Flake8 for code linting and adhere to its rules

Files:

  • packages/opentelemetry-instrumentation-writer/opentelemetry/instrumentation/writer/__init__.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: traceloop/openllmetry#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-17T15:06:48.109Z
Learning: Instrumentation packages must leverage the semantic conventions package and emit OTel-compliant spans
🧬 Code graph analysis (1)
packages/opentelemetry-instrumentation-writer/opentelemetry/instrumentation/writer/__init__.py (5)
packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/__init__.py (2)
  • Meters (36-61)
  • SpanAttributes (64-261)
packages/opentelemetry-instrumentation-writer/opentelemetry/instrumentation/writer/config.py (1)
  • Config (1-3)
packages/opentelemetry-instrumentation-writer/opentelemetry/instrumentation/writer/event_emitter.py (2)
  • emit_choice_events (57-87)
  • emit_message_events (35-53)
packages/opentelemetry-instrumentation-writer/opentelemetry/instrumentation/writer/span_utils.py (4)
  • set_input_attributes (13-56)
  • set_model_input_attributes (60-75)
  • set_model_response_attributes (79-125)
  • set_response_attributes (129-137)
packages/opentelemetry-instrumentation-writer/opentelemetry/instrumentation/writer/utils.py (9)
  • enhance_list_size (148-156)
  • error_metrics_attributes (65-70)
  • initialize_accumulated_response (113-127)
  • initialize_choice (130-139)
  • initialize_tool_call (142-145)
  • request_type_by_method (73-79)
  • response_attributes (83-90)
  • should_emit_events (93-99)
  • wrapper (49-59)
🪛 Ruff (0.12.2)
packages/opentelemetry-instrumentation-writer/opentelemetry/instrumentation/writer/__init__.py

616-631: Use contextlib.suppress(ModuleNotFoundError) instead of try-except-pass

Replace with contextlib.suppress(ModuleNotFoundError)

(SIM105)


637-652: Use contextlib.suppress(ModuleNotFoundError) instead of try-except-pass

Replace with contextlib.suppress(ModuleNotFoundError)

(SIM105)

🔇 Additional comments (1)
packages/opentelemetry-instrumentation-writer/opentelemetry/instrumentation/writer/__init__.py (1)

382-395: Returning the generator and delegating span finalization to the stream processors is correct.

Also applies to: 483-496

@nirga nirga changed the title feat(instrumentation): implementation of Writer opentelemetry-instrumentation-writer custom instrumentation feat(writer): initial implementation Aug 27, 2025
@nirga nirga merged commit fd0c09f into traceloop:main Aug 29, 2025
10 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants