Conversation
Greptile SummaryAdds Key changes:
Note: The Confidence Score: 5/5
Important Files Changed
Last reviewed commit: eeba1bb |
There was a problem hiding this comment.
Pull request overview
This PR adds support for usage_details and cost_details setters to the Generation observation class, deprecating the legacy usage= setter. The change moves away from the old camelCase attribute conversion behavior to a simpler approach that preserves the original key format. All generation-focused documentation has been updated to use the new usage_details API.
Changes:
- Added
Generation#usage_details=andGeneration#cost_details=setters that delegate through the unified observation update path - Deprecated
Generation#usage=with a deprecation warning that forwards tousage_details= - Updated all documentation examples to use
usage_detailsinstead ofusage
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/langfuse/observations.rb | Added new usage_details= and cost_details= setters, deprecated usage= with warning mechanism |
| spec/langfuse/base_observation_spec.rb | Added comprehensive tests for new setters and deprecation behavior |
| docs/TRACING.md | Updated examples to use usage_details instead of usage |
| docs/SCORING.md | Updated examples to use usage_details instead of usage |
| docs/RAILS.md | Updated examples to use usage_details instead of usage |
| docs/PROMPTS.md | Updated examples to use usage_details instead of usage |
| docs/GETTING_STARTED.md | Updated examples to use usage_details instead of usage |
| docs/ARCHITECTURE.md | Updated examples to use usage_details instead of usage |
| docs/API_REFERENCE.md | Added documentation for new setters and deprecation notice |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lib/langfuse/observations.rb
Outdated
| def usage=(value) | ||
| return unless @otel_span.recording? | ||
| warn_usage_deprecation | ||
| self.usage_details = value | ||
| end |
There was a problem hiding this comment.
The Embedding class has a usage= setter (line 681) that silently forwards to usage_details without any deprecation warning, while Generation#usage= logs a deprecation warning. For consistency, the Embedding class should either also have a deprecation warning for its usage= setter, or should have a corresponding usage_details= setter added. This inconsistency could confuse users about which API to use.
lib/langfuse/observations.rb
Outdated
| def usage_details=(value) | ||
| update_observation_attributes(usage_details: value) | ||
| end | ||
|
|
||
| usage_json = usage_hash.to_json | ||
| @otel_span.set_attribute("langfuse.observation.usage", usage_json) | ||
| # @param value [Hash] Cost details hash (e.g., total_cost and provider-specific fields) | ||
| # @return [void] | ||
| def cost_details=(value) | ||
| update_observation_attributes(cost_details: value) | ||
| end |
There was a problem hiding this comment.
The model= and model_parameters= setters directly call @otel_span.set_attribute() and check @otel_span.recording?, while the new usage_details= and cost_details= setters use update_observation_attributes() which internally handles the recording check. This inconsistency in implementation patterns within the same class may lead to maintenance issues. Consider refactoring model= and model_parameters= to also use update_observation_attributes() for consistency, or document why they need different implementation patterns.
lib/langfuse/observations.rb
Outdated
| Langfuse.configuration.logger&.warn( | ||
| "Langfuse::Generation#usage= is deprecated; use #usage_details= instead." | ||
| ) |
There was a problem hiding this comment.
The deprecation warning uses Langfuse.configuration.logger&.warn(...) with the safe navigation operator. This means if the logger is nil, the warning will be silently suppressed. While this is likely intentional for cases where logging isn't configured, consider whether this is the desired behavior. If deprecation warnings should always be visible regardless of logger configuration, you might want to fall back to warn or $stderr.puts when the logger is nil.
| Langfuse.configuration.logger&.warn( | |
| "Langfuse::Generation#usage= is deprecated; use #usage_details= instead." | |
| ) | |
| message = "Langfuse::Generation#usage= is deprecated; use #usage_details= instead." | |
| logger = Langfuse.configuration.logger | |
| if logger | |
| logger.warn(message) | |
| else | |
| warn(message) | |
| end |
| "Langfuse::Generation#usage= is deprecated; use #usage_details= instead." | ||
| ) | ||
| end | ||
|
|
There was a problem hiding this comment.
The test verifies that the deprecation warning is logged when usage= is called, but it doesn't test that the warning is only logged once per instance even if usage= is called multiple times. Consider adding a test case that calls usage= twice and verifies the warning is logged exactly once, to ensure the @usage_deprecation_logged flag works as intended.
| it "logs usage deprecation warning only once per instance" do | |
| allow(Langfuse.configuration.logger).to receive(:warn) | |
| generation.usage = { prompt_tokens: 10, completion_tokens: 5, total_tokens: 15 } | |
| generation.usage = { prompt_tokens: 20, completion_tokens: 10, total_tokens: 30 } | |
| span_data = generation.otel_span.to_span_data | |
| usage_attr_value = span_data.attributes["langfuse.observation.usage_details"] | |
| expect(usage_attr_value).not_to be_nil | |
| usage_attr = JSON.parse(usage_attr_value) | |
| expect(usage_attr["prompt_tokens"]).to eq(20) | |
| expect(usage_attr["completion_tokens"]).to eq(10) | |
| expect(usage_attr["total_tokens"]).to eq(30) | |
| expect(span_data.attributes).not_to have_key("langfuse.observation.usage") | |
| expect(Langfuse.configuration.logger).to have_received(:warn).once.with( | |
| "Langfuse::Generation#usage= is deprecated; use #usage_details= instead." | |
| ) | |
| end |
- Delegate Generation#model= and #model_parameters= to update_observation_attributes, matching the pattern used by usage_details=, cost_details=, and the Embedding class. This also fixes a pre-existing bug where these setters used wrong OTel attribute names (langfuse.observation.model vs langfuse.observation.model.name). - Simplify warn_usage_deprecation by removing unnecessary defined? guard. - Update tests to expect correct OTel attribute names.
- Fall back to Kernel#warn when logger is nil in Generation and Embedding deprecation warnings, ensuring deprecation notices are always visible regardless of logger configuration. - Add deprecation warning to Embedding#usage= for consistency with Generation#usage=, including usage_details= setter and once-per- instance warning guard. - Add test coverage for once-only deprecation warning behavior on both Generation and Embedding classes. - Update Embedding doc example to use usage_details instead of usage.
…ils-and-usage-details-support-on-generations
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
…d Embedding Deduplicates usage=, usage_details=, model=, and model_parameters= setters that were identical across both classes. cost_details= remains on Generation.
kxzk
deleted the
feature/add-cost-details-and-usage-details-support-on-generations
branch
Supersedes #55 after branch rename to conventional format.
TL;DR
Add
usage_details=andcost_details=setters to Generation and Embedding, route all model-related setters throughupdate_observation_attributes, and extract shared setters into aModelSettersmodule.Why
The existing
usage=setter on Generation did inline camelCase key remapping and wrote to a legacylangfuse.observation.usageOTel attribute. This diverged from the JS/Python SDKs which pass usage payload shape as-is viausage_details. Thecost_detailssetter was missing entirely.Changes
usage_details=on Generation and Embedding — preserves key shape as provided, writes tolangfuse.observation.usage_detailscost_details=on Generation — writes tolangfuse.observation.cost_detailsusage=kept as a compatibility alias that forwards tousage_details=(no deprecation warning)model=andmodel_parameters=refactored to route throughupdate_observation_attributesinstead of direct@otel_span.set_attributecallsModelSettersmodule extracts the 4 shared setters (usage=,usage_details=,model=,model_parameters=) included by both Generation and Embeddingusage_detailsin all examplescost_detailsupdated to recommend:input,:output,:totalkeysTest plan
bundle exec rspec— 1198 examples, 0 failures, 98.06% coveragebundle exec rubocop— no offensesusage_details=tested on both Generation and Embeddingcost_details=tested on Generationusage=alias verified to write tousage_detailsattribute (not legacyusage)model=andmodel_parameters=verified against new OTel attribute keysChecklist