Repository navigation
feat: group observability spans by run - #195
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR enhances OTLP/HTTP span exports with stable trace identifiers and richer resource metadata. Trace IDs are now deterministically derived from session and run IDs via SHA-256 hashing (truncated to 32 hex characters), or fall back to random tokens when unavailable. Resource attributes now include ChangesStable OTLP Trace Identifiers and Resource Metadata
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
coding-agent/tests/coding_agent/test_observability.py (2)
91-124: ⚡ Quick winConsider adding test coverage for the random trace ID fallback.
The current test verifies deterministic grouping when both
session_idandrun_idare present. Consider adding a test case that verifies the fallback to random trace IDs when either identifier is missing or invalid, to document this behavior.📝 Example test case
def test_otlp_sink_uses_random_trace_id_without_session_or_run() -> None: transport = RecordingTransport() sink = OtlpHttpObservationSink( endpoint="https://otel.example.test/v1/traces", client=httpx.Client(transport=httpx.MockTransport(transport.handler)), ) # Span without session_id or run_id should get random trace ID sink.record_span( SpanRecord( name="tool.call", status="ok", attributes={"other_attr": "value"}, ) ) sink.record_span( SpanRecord( name="llm.generation", status="ok", attributes={"other_attr": "value"}, ) ) first_trace_id = _first_span(_payload(transport.requests[0]))["traceId"] second_trace_id = _first_span(_payload(transport.requests[1]))["traceId"] # Random trace IDs should be different assert first_trace_id != second_trace_id🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@coding-agent/tests/coding_agent/test_observability.py` around lines 91 - 124, Add a new test that verifies OtlpHttpObservationSink falls back to random trace IDs when session_id/run_id are missing: create a RecordingTransport and OtlpHttpObservationSink, record two SpanRecord instances that do NOT include session_id or run_id in their attributes, then extract trace IDs with _first_span(_payload(transport.requests[i])) and assert the two trace IDs are different; this complements the existing test_otlp_sink_groups_spans_by_session_and_run and uses the same helpers (RecordingTransport, OtlpHttpObservationSink, SpanRecord, _first_span, _payload) for consistency.
127-144: 💤 Low valueConsider verifying the
service.nameresource attribute.The
_resource_attributesfunction always includesservice.namein the resource attributes. Consider adding an assertion to verify its presence and value for completeness.📝 Suggested addition
resource_attributes = _resource_attributes(_payload(transport.requests[0])) + assert resource_attributes["service.name"] == {"stringValue": "coding-agent"} assert resource_attributes["session.id"] == {"stringValue": "session-1"} assert resource_attributes["run.id"] == {"stringValue": "run-1"}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@coding-agent/tests/coding_agent/test_observability.py` around lines 127 - 144, Update the test test_otlp_sink_exports_session_and_run_as_resource_attributes to also assert that the resource attributes include the service.name attribute; after calling resource_attributes = _resource_attributes(_payload(transport.requests[0])) add an assertion that resource_attributes contains "service.name" and either equals the expected service name value used by OtlpHttpObservationSink (as {"stringValue": "<expected>"}), or at minimum that its stringValue is present and non-empty to ensure the service name is exported; locate this change around the existing assertions in that test which call _resource_attributes, _payload, and use the OtlpHttpObservationSink/test transport.coding-agent/src/coding_agent/observability.py (1)
75-85: ⚡ Quick winConsider using a collision-resistant separator strategy.
The current implementation concatenates
session_idandrun_idwith a colon separator before hashing. If either identifier can contain colons, different (session_id, run_id) pairs could produce identical hash inputs, leading to trace ID collisions and incorrect span grouping.For example:
session_id="a:b",run_id="c"→ hash input"a:b:c"session_id="a",run_id="b:c"→ hash input"a:b:c"(collision)While unlikely if IDs are UUIDs or structured identifiers without colons, consider one of these approaches:
♻️ Option 1: Length-prefixed encoding (most robust)
- return hashlib.sha256(f"{session_id}:{run_id}".encode("utf-8")).hexdigest()[:32] + combined = f"{len(session_id)}:{session_id}:{run_id}" + return hashlib.sha256(combined.encode("utf-8")).hexdigest()[:32]♻️ Option 2: Document the constraint
Add a docstring comment:
def _trace_id(attributes: Mapping[str, Any]) -> str: """Generate deterministic trace ID from session and run IDs. Requires session_id and run_id to not contain colon characters. """🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@coding-agent/src/coding_agent/observability.py` around lines 75 - 85, _trace_id currently concatenates session_id and run_id with ":" which can collide if IDs contain colons; change the input to the hash to use a collision-resistant encoding (e.g., length-prefixed encoding or another unambiguous separator) before calling hashlib.sha256 so distinct (session_id, run_id) pairs cannot produce the same preimage; update the logic in _trace_id (and keep the existing secrets.token_hex fallback) so the deterministic branch hashes the new encoded string built from session_id and run_id rather than plain f"{session_id}:{run_id}".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@coding-agent/src/coding_agent/observability.py`:
- Around line 75-85: _trace_id currently concatenates session_id and run_id with
":" which can collide if IDs contain colons; change the input to the hash to use
a collision-resistant encoding (e.g., length-prefixed encoding or another
unambiguous separator) before calling hashlib.sha256 so distinct (session_id,
run_id) pairs cannot produce the same preimage; update the logic in _trace_id
(and keep the existing secrets.token_hex fallback) so the deterministic branch
hashes the new encoded string built from session_id and run_id rather than plain
f"{session_id}:{run_id}".
In `@coding-agent/tests/coding_agent/test_observability.py`:
- Around line 91-124: Add a new test that verifies OtlpHttpObservationSink falls
back to random trace IDs when session_id/run_id are missing: create a
RecordingTransport and OtlpHttpObservationSink, record two SpanRecord instances
that do NOT include session_id or run_id in their attributes, then extract trace
IDs with _first_span(_payload(transport.requests[i])) and assert the two trace
IDs are different; this complements the existing
test_otlp_sink_groups_spans_by_session_and_run and uses the same helpers
(RecordingTransport, OtlpHttpObservationSink, SpanRecord, _first_span, _payload)
for consistency.
- Around line 127-144: Update the test
test_otlp_sink_exports_session_and_run_as_resource_attributes to also assert
that the resource attributes include the service.name attribute; after calling
resource_attributes = _resource_attributes(_payload(transport.requests[0])) add
an assertion that resource_attributes contains "service.name" and either equals
the expected service name value used by OtlpHttpObservationSink (as
{"stringValue": "<expected>"}), or at minimum that its stringValue is present
and non-empty to ensure the service name is exported; locate this change around
the existing assertions in that test which call _resource_attributes, _payload,
and use the OtlpHttpObservationSink/test transport.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f73002c1-993b-462d-8666-d8363e970833
📒 Files selected for processing (2)
coding-agent/src/coding_agent/observability.pycoding-agent/tests/coding_agent/test_observability.py
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Verification
Summary by CodeRabbit