-
Notifications
You must be signed in to change notification settings - Fork 189
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
Included Prometheus interceptor support for gRPC streaming #1858
Included Prometheus interceptor support for gRPC streaming #1858
Conversation
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.
LGTM. I left some minor comments.
@@ -38,14 +38,14 @@ def _create_server(self): | |||
self._model_repository_handlers | |||
) | |||
|
|||
interceptors = [] | |||
self._interceptors = [] |
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.
why are we changing it, was that a bug?
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.
Was not a bug. Just needed a way to access the interceptors list for testing.
@@ -0,0 +1,129 @@ | |||
import pytest |
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.
thanks for adding these tests. ideally we also should test other metrics / errors but can happen as a follow up PR.
async def get_stream_request(request): | ||
yield request | ||
|
||
# send 10 requests |
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.
# send 10 requests | |
# send 1 requests |
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.
That should be 10, but I forgot to update the num_request
var.
num_words = len(request_text.split()) | ||
|
||
assert int(counted_requests) == num_requests | ||
assert int(counted_requests) * num_words == int(counted_responses) |
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.
question: how are we actually counting the actual words in the responses?
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.
The model used is a dummy text one which split words by white space. Each word is going to be streamed back by the model.
0afebdc
to
55a271e
Compare
This PR includes Prometheus interceptor support for gRPC streaming. Currently for gRPC streaming, we have to set
"metrics_endpoint": null
, thus Prometheus logs cannot be scraped. It also updates docs and test for Prometheus interceptor.