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

Test refactor and fix client metrics #8

Merged
merged 6 commits into from
Nov 26, 2024

Conversation

artificial-aidan
Copy link

No description provided.

* Add typing
* Refactor proto gen to generate typing and all test files
* Standardize test stub creation using random ports

Signed-off-by: Aidan Jensen <[email protected]>
Move observation of client duration to after the code coroutine. The continuation
coroutine doesn't actually wait for the call to complete

Signed-off-by: Aidan Jensen <[email protected]>
@artificial-aidan
Copy link
Author

artificial-aidan commented Nov 21, 2024

@avik-taranis I found that the continuation did not actually wait for the call to finish, which is different than the sync version. So i moved it to after the code await, and its working now.

I also had issues running the tests on the default ports, so I moved to a pattern of letting the server pick an unused port.

@artificial-aidan
Copy link
Author

I had to make a bunch of other changes to make the rest of the client interceptors work, including changing how they are instantiated because of this issue: grpc/grpc#31442

@avik-taranis
Copy link

@artificial-aidan thanks for the PR!

  1. Can you please update the Readme with how to use the client interceptor? using get_client_interceptors.
  2. What about the exception test in test_grpc_client.py? Is it relevant for the client interceptor as well?
  3. In test_grpc_client.py - I would add a comment explaining why you use get_server_metric as it's confusing because the tests are testing client metrics (pointing to _grpc_stub starting the metrics server). Or change the function name to get_metrics.

Signed-off-by: Aidan Jensen <[email protected]>
Signed-off-by: Aidan Jensen <[email protected]>
@artificial-aidan
Copy link
Author

@artificial-aidan thanks for the PR!

  1. Can you please update the Readme with how to use the client interceptor? using get_client_interceptors.
  2. What about the exception test in test_grpc_client.py? Is it relevant for the client interceptor as well?
  3. In test_grpc_client.py - I would add a comment explaining why you use get_server_metric as it's confusing because the tests are testing client metrics (pointing to _grpc_stub starting the metrics server). Or change the function name to get_metrics.
  1. Done
  2. I don't think it is relevant, because the metrics should still get recorded
  3. I renamed the get_server_metrics to get_metrics

@avik-taranis
Copy link

@razsinay @shay-taranis can you take a look as well?

registry=REGISTRY,
):
metrics = init_metrics(REGISTRY)
_PromAsyncUnaryUnaryClientInterceptor.__init__(

Choose a reason for hiding this comment

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

Why not do _PromAsyncUnaryUnaryClientInterceptor() etc in this case?

Copy link
Author

Choose a reason for hiding this comment

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

Because this is instantiating each of the parent classes. It would be like calling super().__init__(...) if there was only one parent class. And super works differently in multiple inheritance.

grpc_service=grpc_service_name,
grpc_method=grpc_method_name).inc()
yield item
if timer:

Choose a reason for hiding this comment

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

Why is the timer necessary?

Copy link
Author

Choose a reason for hiding this comment

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

The timer is to record the total time the call took for the server streaming to finish. I'm trying for parity with go interceptors: https://github.com/grpc-ecosystem/go-grpc-middleware/blob/main/interceptors/client.go

As I read this, the timer finishes after the EOF is sent, which is the same as when the iterator would stop iterating in python.

@avik-taranis avik-taranis merged commit 1a3443e into taranisag:master Nov 26, 2024
6 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.

3 participants