-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Enhance OTEL testing to capture and verify Cancellation Requests and Non-Decoupled model inference. #7132
Conversation
Could you please add a description, clarifying what tests were added? |
Could you please attach a picture of a trace as displayed in jaeger with cancelled request. Another question, have you considered cases when request was cancelled, when it was in a queue and when it was already in a compute stage? |
…erver/server into indrajit_otel_testing
Can you update the PR title to be more descriptive? (cancellation, decoupled responses, etc. rather than JIRA ticket number) |
@@ -339,6 +389,22 @@ def _verify_headers_propagated_from_client_if_any(self, root_span, headers): | |||
), | |||
) | |||
|
|||
def _test_trace_cancel(self, is_queued): | |||
# TO accommodate for delay in the model |
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.
Could you please clarify what delay in the model? and if it is a model related, why are we using COLLECTOR_TIMEOUT
variable?
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.
We want to capture a cancellation request traces WHILE the inference is in the COMPUTE stage.
Because the model "input_all_required" has a delay/wait in the compute phase so the cancellation request can be send while the request is waiting in the compute phase.
The idea here is to wait before we try and read the traces from the file.
If we do not wait, the file in empty without traces.
Updated the comments in the test to reflect his better.
I think it's in a good state. Let's address Ryan's comment and provide a clear description for 3 tests, added in this PR, to the PR description |
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! Thank you for this hard work!
Added tests for
Tests added