Skip to content
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

Merged

Conversation

RobertSamoilescu
Copy link
Contributor

@RobertSamoilescu RobertSamoilescu commented Jul 12, 2024

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.

Copy link
Member

@sakoush sakoush left a 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.

mlserver/grpc/interceptors.py Outdated Show resolved Hide resolved
@@ -38,14 +38,14 @@ def _create_server(self):
self._model_repository_handlers
)

interceptors = []
self._interceptors = []
Copy link
Member

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?

Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# send 10 requests
# send 1 requests

Copy link
Contributor Author

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.

mlserver/grpc/interceptors.py Show resolved Hide resolved
mlserver/grpc/interceptors.py Outdated Show resolved Hide resolved
num_words = len(request_text.split())

assert int(counted_requests) == num_requests
assert int(counted_requests) * num_words == int(counted_responses)
Copy link
Member

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?

Copy link
Contributor Author

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.

@RobertSamoilescu RobertSamoilescu merged commit ce93346 into SeldonIO:master Jul 15, 2024
26 checks passed
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.

2 participants