Skip to content

Fix single embed_sparse and embed_all gRPC request metrics - #919

Open
shoemoney wants to merge 1 commit into
huggingface:mainfrom
shoemoney:fix-grpc-single-embed-metrics
Open

Fix single embed_sparse and embed_all gRPC request metrics#919
shoemoney wants to merge 1 commit into
huggingface:mainfrom
shoemoney:fix-grpc-single-embed-metrics

Conversation

@shoemoney

Copy link
Copy Markdown

What does this PR do?

The gRPC embed_sparse and embed_all handlers increment te_request_count{method="single"} twice per request, once before inference and again after a successful response, and never record te_request_success{method="single"}. One successful request therefore counts as two, and a success/error ratio derived from these counters reports a permanent 100% failure rate for sparse and all-embedding gRPC traffic.

This PR records the post-success event as te_request_success instead, at router/src/grpc/server.rs:647 and router/src/grpc/server.rs:689 on main. The sibling paths already implement the count then success pair correctly: the same file's embed method (grpc/server.rs 595/606) and the HTTP embed handlers (http/server.rs 603/621, 792/808, 970/986, 1174/1192). #917 fixes the identical double-increment in the HTTP /predict single arm and its description explicitly leaves the gRPC paths out of scope; this PR covers that named remainder.

Following the pattern of #917, the two counter calls move into record_single_request/record_single_success helpers with a recorder-backed unit test that needs no model, network, or accelerator. With the buggy counter name in place the test fails with count=2 and success=0; with this fix it passes. The already-correct embed, predict, rerank, and batch call sites are intentionally unchanged to keep the diff minimal.

Related to #879.

Validation

  • cargo test -p text-embeddings-router --no-default-features --features grpc,candle --lib (1 passed)
  • rustfmt --check router/src/grpc/server.rs --edition 2021
  • cargo clippy -p text-embeddings-router --no-default-features --features grpc,candle --lib (passes; only pre-existing warnings, including result_large_err on the untouched auth closure at grpc/server.rs:1484)

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline?
  • Was this discussed/approved via a GitHub issue or the forum? Tracked in te_request_count (method=single) is incremented twice for single requests #879.
  • Did you make sure to update the documentation with your changes? No documentation change is needed for this metrics correction.
  • Did you write any new necessary tests? If applicable, did you include or update the insta snapshots? Added a recorder-backed unit test; no snapshot changes needed.

gRPC embed_sparse and embed_all incremented te_request_count
{method="single"} twice per request, once before inference and again
after a successful response, and never recorded te_request_success
{method="single"}. One successful request therefore counted as two, and
a success/error ratio derived from these counters reported a permanent
100% failure rate for sparse and all-embedding gRPC traffic.

Record the post-success event as te_request_success instead, matching
the sibling paths that already implement the count then success pair
correctly: the same file's embed method (grpc/server.rs:595 count, 606
success) and all HTTP embed handlers (http/server.rs 603/621, 792/808,
970/986, 1174/1192). Open PR huggingface#917 fixes the identical double-increment
in the HTTP /predict single arm and its body explicitly states "Batch
and gRPC paths are also outside this fix", naming the sibling it did
not fix.

The counter calls move into record_single_request and
record_single_success helpers with a recorder-backed unit test that
needs no model or network, the same pattern PR huggingface#917 uses.

Related to huggingface#879.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant