Conversation
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
Pull request overview
Adds deterministic/custom trace ID support to the Ruby SDK (matching Python/JS), wires trace_id: into start_observation/observe, and hardens block-based observation APIs so spans reliably end via ensure even when exceptions are raised.
Changes:
- Introduces
Langfuse::TraceIdhelpers for deterministic/random trace and observation ID generation plus SpanContext conversion. - Adds
Langfuse.create_trace_id/Langfuse.create_observation_idconvenience APIs andtrace_id:support onstart_observationandobserve(mutually exclusive withparent_span_context:). - Refactors block execution for observations to use
ensurefor reliable span end behavior on exceptions, with added specs.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/langfuse/trace_id_spec.rb | Adds unit coverage for deterministic ID generation/validation and SpanContext conversion. |
| spec/langfuse/base_observation_spec.rb | Adds regression spec ensuring child spans end and exceptions re-raise. |
| spec/langfuse_spec.rb | Adds coverage for new convenience APIs, trace_id: behavior, exclusivity checks, and ensure semantics in observe. |
| lib/langfuse/trace_id.rb | Implements deterministic/random ID helpers, validation, and to_span_context. |
| lib/langfuse/observations.rb | Refactors child block execution to ensure spans end on exceptions. |
| lib/langfuse.rb | Wires trace_id: into start_observation/observe, adds convenience methods, and introduces a trace context resolver. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def valid?(trace_id) | ||
| trace_id.is_a?(String) && TRACE_ID_PATTERN.match?(trace_id) | ||
| end |
There was a problem hiding this comment.
valid? currently only checks the hex format. Per the W3C trace-context spec, a trace ID of all zeros ("0" * 32) is invalid and may be treated as an invalid SpanContext by OpenTelemetry. Consider updating validation to also reject the all-zero trace ID to ensure to_span_context behaves consistently.
| # @param id [Object] Value to validate | ||
| # @return [Boolean] true when the value is a 16-char lowercase hex string | ||
| def valid_observation_id?(id) | ||
| id.is_a?(String) && OBSERVATION_ID_PATTERN.match?(id) | ||
| end |
There was a problem hiding this comment.
valid_observation_id? currently accepts any 16-char lowercase hex string, including the all-zero span ID ("0" * 16) which is invalid under the W3C trace-context spec. Tightening validation here will prevent callers from generating/accepting span IDs that OpenTelemetry may treat as invalid.
lib/langfuse/observations.rb
Outdated
| # Events are excluded because they auto-end at creation. | ||
| # | ||
| # @api private | ||
| def execute_child_block(child, as_type, &block) |
There was a problem hiding this comment.
execute_child_block is tagged as @api private but is currently a public instance method on BaseObservation, expanding the public API surface. Consider making it private (e.g., private def execute_child_block ...) so only start_observation exposes the intended API.
| def execute_child_block(child, as_type, &block) | |
| private def execute_child_block(child, as_type, &block) |
lib/langfuse.rb
Outdated
| # guaranteeing the span ends via ensure — even if the block raises. | ||
| # Events are excluded because they auto-end in {start_observation}. | ||
| # | ||
| # @api private | ||
| def execute_observe_block(observation, as_type, &block) |
There was a problem hiding this comment.
execute_observe_block is documented as @api private but is currently a public module method (it’s defined before the private section). Consider making it private (move it below private or declare private :execute_observe_block) to avoid exposing an internal helper as part of the public API.
…vation Post-review cleanup on ML-964: - Deduplicate execute_observe_block / execute_child_block into a single Langfuse.run_in_observation_context; BaseObservation#start_observation now delegates to it - Extract validate_observation_type! and apply_observation_attributes private helpers; drop the Metrics/AbcSize rubocop disable on start_observation (only ParameterLists remains) - Rewrite TraceId.to_span_context YARD to accurately describe how the synthetic span_id is consumed - Trim narrative comments per CLAUDE.md (WHY, not WHAT)
- Reject the W3C all-zero trace_id and span_id in TraceId.valid? / valid_observation_id? so OpenTelemetry never sees an invalid SpanContext. - Document the new trace_id: parameter on Langfuse.start_observation and Langfuse.observe with @param/@raise YARD tags. - Move run_in_observation_context behind `private` on the Langfuse module so the internal helper isn't part of the public API; BaseObservation reaches it via __send__.
TL;DRAdd
Langfuse::TraceIdfor deterministic/custom trace IDs and wiretrace_id:intostart_observation/observe, matching the Python and JS SDKs.WhyUsers need to correlate external system IDs (DB primary keys, request IDs) with Langfuse traces — e.g. to score a trace later from a background job or link traces across services. An external PR (#69) attempted this but had bugs (regex anchors, implicit nil var, rubocop disables) and lacked the deterministic
create_trace_id(seed:)helper both reference SDKs expose. This change implements the reference SDK surface cleanly and fixes a pre-existing ensure-block bug onobserve/BaseObservation#start_observationdiscovered along the way.What's in the change
Langfuse::TraceIdmodule —.create(seed:),.create_observation_id(seed:),.valid?,.valid_observation_id?,.to_span_context. Uses\A/\zanchors andSHA-256matching the Python/JS SDKs so the same seed produces the same trace ID across all three.Langfuse.create_trace_id(seed:)/Langfuse.create_observation_id(seed:)flat-API convenience methods.trace_id:keyword onLangfuse.start_observationandLangfuse.observe; mutually exclusive withparent_span_context:. Invalid IDs raiseArgumentError.Langfuse.observeandBaseObservation#start_observation— exceptions in the block now reliably end the span viaensureand re-raise.id, observations (2), score (1) and tags all present.ChecklistCloses ML-964