fix(signer): meter real BYOC capability + model_id for usage events#3966
fix(signer): meter real BYOC capability + model_id for usage events#3966seanhanca wants to merge 3 commits into
Conversation
The remote signer hardcoded the create_signed_ticket `pipeline` to the lv2v constant whenever the gateway sent `type:"lv2v"` (which BYOC jobs always do for fee/pixel routing), and derived `model_id` only from the LiveVideoToVideo capability constraints. For BYOC capabilities (e.g. nano-banana) this emitted pipeline=live-video-to-video and an empty model_id, which the OpenMeter collector recorded as live-video-to-video / unknown. Add two additive, backward-compatible fields to RemotePaymentRequest: `capability` and `model_id`. When set by the gateway, they override the metered pipeline + model_id labels, decoupling usage attribution from `Type` (which still drives fee/pixel routing). When empty, behavior is byte-identical to before (lv2v constant + capabilities-derived model id; collector defaults empties to "unknown"). Label resolution is extracted into a pure resolveUsageLabels helper with a hermetic table test (TestResolveUsageLabels) that runs without the CGO ffmpeg toolchain.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR fixes usage metering attribution for remote-signer BYOC jobs by allowing the gateway to supply the true capability and model ID, instead of always emitting pipeline=live-video-to-video / model_id=unknown due to Type=="lv2v" fee-routing semantics.
Changes:
- Added optional
capabilityandmodel_idfields toRemotePaymentRequestto support BYOC attribution overrides. - Extracted metering label selection into a
resolveUsageLabels(req, caps)helper and used it in thecreate_signed_ticketusage event emission. - Added a hermetic table-driven unit test for
resolveUsageLabels.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
server/remote_signer.go |
Adds request fields and factors out metering label resolution so BYOC can override pipeline/model_id while keeping lv2v fee routing intact. |
server/remote_signer_test.go |
Adds table-driven unit coverage for the new label-resolution helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // pipeline + model. When those additive fields are empty, the labels fall back | ||
| // to the previous behavior: for lv2v the pipeline is the live-video-to-video | ||
| // constant and the model id is derived from the request capabilities; for any | ||
| // other request both remain empty (the collector then defaults them to | ||
| // "unknown"). This makes non-BYOC (lv2v) callers byte-identical to before and | ||
| // keeps usage attribution decoupled from `Type`, which still drives fee/pixel | ||
| // routing. |
There was a problem hiding this comment.
Fixed in f7d2f83. Reworded the docstring: the req.Capability/req.ModelID overrides apply whenever set, regardless of Type (decoupled from fee/pixel routing), and clarified the empty-field fallback. Behavior is unchanged.
| if req.Capability != "" { | ||
| pipeline = req.Capability | ||
| } | ||
| if req.ModelID != "" { | ||
| modelID = req.ModelID | ||
| } |
There was a problem hiding this comment.
Good call — addressed in f7d2f83. Added sanitizeUsageLabel (TrimSpace + cap at 128 runes) applied to the gateway-supplied Capability/ModelID before they become the pipeline/model_id labels, bounding Kafka payload size and label cardinality. I went with a length cap rather than a strict allowlist because the whole point of BYOC is to support arbitrary new capabilities/models without a code change; 128 runes is generous for real identifiers while still bounding abuse. Added whitespace-only (ignored) and oversized (capped) test cases.
| func TestResolveUsageLabels(t *testing.T) { | ||
| require := require.New(t) | ||
|
|
There was a problem hiding this comment.
Fixed in f7d2f83. TestResolveUsageLabels now creates a fresh require.New(t) inside each t.Run closure so failures are attributed to the specific subtest.
| // `Type` (which stays "lv2v" for fee/pixel routing). Optional; empty | ||
| // preserves the previous behavior (pipeline falls back to the lv2v | ||
| // constant when Type=="lv2v"). | ||
| Capability string `json:"capability,omitempty"` |
There was a problem hiding this comment.
I'd want to try to avoid changing the format of RemotePaymentRequest if we can. There is a bytearray of capabilities already supported in the request.
This looks like a familiar byoc problem. It reminds me of what I resolved in an earlier PR #3869 that was never merged. The primary difference in how that PR handles capability and constraints in the new stream request is the older PR has a new /sign-byoc-job endpoint mirroring /generate-live-payment. The extra endpoint likely wasn't necessary, but the way that capability and constraint selection was done for BYOC jobs is correct. See also matching python-gateway changes livepeer/livepeer-python-gateway#6
There was a problem hiding this comment.
Thanks @eliteprox — good context on #3869 / the /sign-byoc-job approach and python-gateway#6.
A few reasons I went with the two additive fields here rather than reusing the existing Capabilities []byte blob:
- That field is a protobuf-encoded
net.Capabilitiesused for ticket capability/max-price selection (streamParams.Capabilities,GetCapabilitiesMaxPrice,ModelIDForCapability). It carries capability IDs + constraints, not the BYOC capability string the gateway routes on (e.g.nano-banana), and it doesn't carry the providermodel_idat all — so it can't supply the two metering labels we need without also changing how that blob is produced/parsed. Capability/ModelIDareomitemptyand purely additive: an older signer ignores them and an empty value is byte-identical to today.Typestayslv2vso fee/pixel routing is untouched — this PR only fixes thecreate_signed_ticketusage labels (previouslylive-video-to-video/unknownfor every BYOC job).- I kept it to request fields rather than a new
/sign-byoc-jobendpoint to stay minimal and avoid a second signing surface, but I'm very happy to align with the feat(remote_signer): Add job token signing endpoint for byoc job types #3869 pattern if you'd prefer that as the long-term shape — especially if BYOC capability/constraint selection (not just metering) is going to move onto the signer. Let me know which direction you want and I'll follow up.
| pipeline = PipelineLiveVideoToVideo | ||
| modelID = streamParams.Capabilities.ModelIDForCapability(core.Capability_LiveVideoToVideo) | ||
| } | ||
| pipeline, modelID := resolveUsageLabels(&req, streamParams.Capabilities) |
There was a problem hiding this comment.
I do like this change, I think supporting different specific, validated capabilities in the request is the right way to go.
https://github.com/seanhanca/go-livepeer/blob/692f79783ad3a825025feeb297b74cba36da50bc/core/capabilities.go#L134
The primary constraint is the way different pipelines are charged
There was a problem hiding this comment.
Agreed — and the per-pipeline charging constraint you flag is exactly what the stacked follow-up #3967 tackles: it resolves the BYOC fee from oInfo.CapabilitiesPrices keyed on req.Capability and bills compute-seconds at that per-capability rate (gated behind the default-OFF -byocPerCapPricing flag), instead of the flat lv2v-synthesized-pixel fee. This PR keeps charging identical and only fixes the metering labels; #3967 is where the pricing-per-capability lands.
Addresses Copilot review feedback on PR livepeer#3966: - resolveUsageLabels now sanitizes the gateway-supplied capability/model_id override labels (TrimSpace + cap at 128 runes via sanitizeUsageLabel) before they are emitted to the create_signed_ticket usage event, bounding Kafka payload size and downstream label cardinality. req.Capability/req.ModelID come from the request body, so an unbounded value could otherwise inflate cardinality (pipeline was previously a small fixed set). - Corrected the resolveUsageLabels docstring: the capability/model_id overrides apply regardless of Type, not only for lv2v; clarified the empty-field fallback wording. - TestResolveUsageLabels now constructs a fresh require.New(t) inside each subtest so failures are attributed to the correct subtest, and adds cases for whitespace-only (ignored) and oversized (length-capped) override labels.
|
Thanks for this. Couple notes:
AFAIK the BYOC code is going to be phased out of the SDK and there is an intention to move the agent to the live runner, so I'm a little reluctant to add fields that will be almost immediately deprecated. It's hard to remove stuff from an API once it's added. |
Problem
The remote signer's
create_signed_ticketmetering event recordedpipeline=live-video-to-video/model_id=unknownfor every BYOCcapability (e.g.
nano-banana), so OpenMeter could not attribute BYOC usage tothe real pipeline + model.
Root cause (model attribution lost upstream at the gateway)
The gateway (
livepeer-python-gateway→byoc.py→_create_byoc_payment)hardcodes
type:"lv2v"for every BYOC job (it needs lv2v fee/pixel routing —BYOC has no per-job pixel count) and sent the real capability only as a bare
JSON field this signer dropped, with no model id. As a result
GenerateLivePaymenthere:emitted
pipeline=live-video-to-video(becauseType=="lv2v") and an emptymodel_id(because BYOC capabilities aren'tCapability_LiveVideoToVideo).The additive wire contract
Two optional string fields are added to
RemotePaymentRequest(POST
/generate-live-payment):capabilitynano-banana)model_idWhen set, they override the metered
pipeline+model_idlabels,decoupling usage attribution from
Type(which still drives fee/pixel routing).When empty/absent, behavior is byte-identical to before: lv2v falls back to
the
PipelineLiveVideoToVideoconstant and the capabilities-derived model id;non-lv2v stays empty (the collector defaults empties to
"unknown"). Thegateway side that sends these fields is the cross-linked PR below.
Changes
CapabilityandModelID(bothomitempty) toRemotePaymentRequest(
server/remote_signer.go).resolveUsageLabels(req, caps)helperand call it from
GenerateLivePayment's emit block. Generic acrosscapabilities (not only
Capability_LiveVideoToVideo).Zero-regression
pipeline/model_idare identical to theprevious logic ⇒ byte-identical event for lv2v / non-BYOC callers.
server/live_payment.go) does notset the new fields, so it is unaffected (
omitempty).Test plan
TestResolveUsageLabels(added toserver/remote_signer_test.go) is ahermetic table test asserting:
live-video-to-video+ caps-derived/empty model(unchanged);
pipeline+model_id(override);End-to-end verification recipe
With this signer change and the gateway change deployed: drive a
nano-bananageneration against the preview chain, then query OpenMetergroupBy=pipeline_model— it should show the real pipeline + model id, notlive-video-to-video/unknown.⚙️ Deployment note (which build must incorporate this for prod)
The signer DMZ + simple-infra signer are deployed from the Docker image
livepeer/go-livepeer:feat-add-model-id-signer-kafka, built from thefeat/add-model-id-signer-kafkabranch (the pymthousedocker/signer-dmz/Dockerfile*andscripts/build-local-signer.shpin thistag). This PR therefore targets that branch so the fix reaches production with
an image rebuild. For prod to be fixed, the
livepeer/go-livepeer:feat-add-model-id-signer-kafkaimage must be rebuiltfrom this branch after merge and the signer redeployed. (
mastercarries anolder variant of the emit block without
model_id; it can receive theequivalent change separately, but it is not what is deployed.)
Cross-link
Gateway side (sends these fields):
livepeer/livepeer-python-gateway#33
Does not modify pymthouse or NaaP; the collector/read
unknownfallbacks areleft in place as safety nets.
Made with Cursor