-
Notifications
You must be signed in to change notification settings - Fork 223
fix(signer): meter real BYOC capability + model_id for usage events #3966
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -247,6 +247,20 @@ type RemotePaymentRequest struct { | |
|
|
||
| // Capabilities to include in the ticket. Optional; may be set for the lv2v job type. | ||
| Capabilities []byte `json:"capabilities"` | ||
|
|
||
| // Capability is the real BYOC capability name (e.g. "nano-banana"). When | ||
| // set, it overrides the metered `pipeline` label for the emitted | ||
| // create_signed_ticket usage event, decoupling usage attribution from | ||
| // `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"` | ||
|
|
||
| // ModelID is the specific provider model from the request | ||
| // payload/parameters. When set, it overrides the metered `model_id` | ||
| // label. Optional; empty preserves the previous behavior (the | ||
| // capabilities-derived model id, which defaults to "unknown" downstream). | ||
| ModelID string `json:"model_id,omitempty"` | ||
| } | ||
|
|
||
| // Returned by the remote signer and includes a new payment plus updated state. | ||
|
|
@@ -355,6 +369,54 @@ func (ls *LivepeerServer) authLivePayment(r *http.Request, state *RemotePaymentS | |
| return *webhookResp.Status, &webhookResp, errors.New(webhookResp.Reason) | ||
| } | ||
|
|
||
| // maxUsageLabelLen bounds the length of gateway-supplied usage labels | ||
| // (pipeline/model_id) before they are emitted to the metering pipeline. | ||
| // req.Capability / req.ModelID come straight from the request body, so a buggy | ||
| // or malicious caller could otherwise send very long / high-cardinality values | ||
| // that inflate Kafka payload size and downstream label cardinality (the | ||
| // pipeline label was previously a small fixed set). 128 runes is generous for | ||
| // real capability/model identifiers. | ||
| const maxUsageLabelLen = 128 | ||
|
|
||
| // sanitizeUsageLabel trims surrounding whitespace and caps the length (in | ||
| // runes, to never emit invalid UTF-8) of a gateway-supplied metering label. | ||
| func sanitizeUsageLabel(s string) string { | ||
| s = strings.TrimSpace(s) | ||
| if r := []rune(s); len(r) > maxUsageLabelLen { | ||
| return string(r[:maxUsageLabelLen]) | ||
| } | ||
| return s | ||
| } | ||
|
|
||
| // resolveUsageLabels derives the metering `pipeline` and `model_id` labels for | ||
| // the emitted create_signed_ticket usage event. | ||
| // | ||
| // The real BYOC capability/model supplied by the gateway (req.Capability / | ||
| // req.ModelID) take precedence and override the labels whenever they are set, | ||
| // regardless of `Type` — this decouples usage attribution from `Type`, which | ||
| // still drives fee/pixel routing. The gateway-supplied values are sanitized | ||
| // (trimmed + length-capped) to bound payload size and label cardinality. | ||
| // | ||
| // 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 the | ||
| // untouched label(s) remain empty (the collector then defaults them to | ||
| // "unknown"). This makes non-BYOC (lv2v) callers with no override | ||
| // byte-identical to before. | ||
| func resolveUsageLabels(req *RemotePaymentRequest, caps *core.Capabilities) (pipeline, modelID string) { | ||
| if req.Type == RemoteType_LiveVideoToVideo { | ||
| pipeline = PipelineLiveVideoToVideo | ||
| modelID = caps.ModelIDForCapability(core.Capability_LiveVideoToVideo) | ||
| } | ||
| if c := sanitizeUsageLabel(req.Capability); c != "" { | ||
| pipeline = c | ||
| } | ||
| if m := sanitizeUsageLabel(req.ModelID); m != "" { | ||
| modelID = m | ||
| } | ||
| return pipeline, modelID | ||
| } | ||
|
|
||
| // GenerateLivePayment handles remote generation of a payment for live streams. | ||
| func (ls *LivepeerServer) GenerateLivePayment(w http.ResponseWriter, r *http.Request) { | ||
| requestID := string(core.RandomManifestID()) | ||
|
|
@@ -680,12 +742,7 @@ func (ls *LivepeerServer) GenerateLivePayment(w http.ResponseWriter, r *http.Req | |
| if state.SequenceNumber == 0 { | ||
| sessionStatus = "new" | ||
| } | ||
| pipeline := "" | ||
| modelID := "" | ||
| if req.Type == RemoteType_LiveVideoToVideo { | ||
| pipeline = PipelineLiveVideoToVideo | ||
| modelID = streamParams.Capabilities.ModelIDForCapability(core.Capability_LiveVideoToVideo) | ||
| } | ||
| pipeline, modelID := resolveUsageLabels(&req, streamParams.Capabilities) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do like this change, I think supporting different specific, validated capabilities in the request is the right way to go. The primary constraint is the way different pipelines are charged
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed — and the per-pipeline charging constraint you flag is exactly what the stacked follow-up #3967 tackles: it resolves the BYOC fee from |
||
| // NB: This could could drop events if tha Kafka queue is full! | ||
| monitor.SendQueueEventAsync("create_signed_ticket", map[string]interface{}{ | ||
| "session_id": state.StateID, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd want to try to avoid changing the format of
RemotePaymentRequestif 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-jobendpoint 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#6There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @eliteprox — good context on #3869 / the
/sign-byoc-jobapproach and python-gateway#6.A few reasons I went with the two additive fields here rather than reusing the existing
Capabilities []byteblob: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)./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.