fix(tracing): migrate RemoteRolloutProcessor to /traces endpoint#444
Closed
fix(tracing): migrate RemoteRolloutProcessor to /traces endpoint#444
Conversation
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit aa476f0. Configure here.
0c2f7db to
0655f89
Compare
Contributor
Author
|
Closing in favor of a server-side compatibility shim. Plan: update the tracing gateway so both |
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
The
/v1/traces/pointwiseendpoint on the Fireworks tracing proxy was removed in favor of a unified/v1/tracesendpoint. This adaptsFireworksTracingAdapter(and its default data loader) to the new contract soRemoteRolloutProcessorkeeps working.What changed
Wire changes on the adapter (
eval_protocol/adapters/fireworks_tracing.py)/v1/traces/pointwise→/v1/traces(both the default path and theproject_id-scoped variant).tags=rollout_id:<id>as the scoping mechanism; the new one expectsrollout_idas a top-level query parameter.get_evaluation_rowsstill accepts the existingtags=[...]kwarg so callers don't have to change — it just pulls therollout_id:<id>entry out and promotes it to a dedicated param, and raisesValueErrorif none is supplied.Input,Output,Tags,InsertionId) instead of the old nested snake-case shape that had anobservations[]array. The converter now reads the new keys directly and drops the "fall back to last GENERATION observation" branch, which has no equivalent on the new endpoint.session_data["langfuse_trace_id"]is now sourced fromInsertionId(the per-LLM-call id on the new row model). The key name is kept so downstream consumers that read it keep working.Default data loader (
eval_protocol/pytest/tracing_utils.py)limit=1instead ofmax_retries=5.update_row_with_remote_traceonly consumes one row and raises if multiple come back, so fetching more is just wasted bytes.max_retries=5was a no-op retry knob specific to the old polling path, which the new endpoint doesn't expose.Change Overview
flowchart LR caller[RemoteRolloutProcessor] --> loader[default_fireworks_output_data_loader] loader --> adapter[FireworksTracingAdapter.get_evaluation_rows] adapter -- old: tags=rollout_id:X --> old[[/v1/traces/pointwise<br/>snake_case + observations[]]] adapter -- new: rollout_id=X, limit=1 --> new[[/v1/traces<br/>PascalCase flat rows]] new --> conv[convert_trace_dict_to_evaluation_row<br/>reads Input/Output/Tags/InsertionId] conv --> row[EvaluationRow] style old stroke-dasharray: 4 2,opacity:0.4Testing
/chat/completionswith a fresh rollout id, then calladapter.get_evaluation_rows(tags=[\"rollout_id:<id>\"], limit=1)and confirm the returnedEvaluationRowhas the expected messages +execution_metadata.rollout_id.Risk / Compatibility
tags=[\"rollout_id:<id>\"]intoget_evaluation_rowskeeps working; the adapter internally rewrites to the new query param.[\"production\"], etc.) and the server would honor them. The new endpoint is rollout-scoped, so the adapter now raisesValueErroriftagsdoesn't include arollout_id:<id>entry. I don't think there are any callers relying on the generic-tag path —update_row_with_remote_traceis the only downstream consumer in this SDK and it always scopes by rollout — but flagging it here so it's explicit.get_evaluation_rowsagainst a legacy tracing backend that still returned that shape. All supportedtracing.fireworks.aideployments have moved to the new endpoint.Made with Cursor
Note
Low Risk
Low risk because the diff as provided is empty and does not modify runtime behavior. If this is unintended, the main risk is missing the expected tracing endpoint migration.
Overview
No changes are present in the supplied diff (
+++ /dev/nullonly), so this PR appears to be a no-op.If an endpoint migration was intended, reviewers should re-check that the actual diff includes the expected updates (e.g.,
FireworksTracingAdapter.get_evaluation_rowsand the default tracing data loader).Reviewed by Cursor Bugbot for commit 0655f89. Bugbot is set up for automated code reviews on this repo. Configure here.