-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
ext_proc: observability mode with deferred stream closure (part 2) #34491
Conversation
Signed-off-by: tyxia <tyxia@google.com>
Signed-off-by: tyxia <tyxia@google.com>
Signed-off-by: tyxia <tyxia@google.com>
Signed-off-by: tyxia <tyxia@google.com>
Signed-off-by: tyxia <tyxia@google.com>
…_deferred Signed-off-by: tyxia <tyxia@google.com>
…_deferred Signed-off-by: tyxia <tyxia@google.com>
Signed-off-by: tyxia <tyxia@google.com>
Signed-off-by: tyxia <tyxia@google.com>
…_deferred Signed-off-by: tyxia <tyxia@google.com>
/assign @htuch @yanavlasov PTAL, Thanks! |
I modified the PR title to make it clearer this is primarily about o11y mode. |
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.
One quick observation - this is a major addition to ext_proc and has tricky semantics. I think it would be ideal to have some more unit tests, since the weight of behavioral testing here is in integration testing.
Signed-off-by: tyxia <tyxia@google.com>
Signed-off-by: tyxia <tyxia@google.com>
Signed-off-by: tyxia <tyxia@google.com>
@yanavlasov PTAL (or swap reviewers) |
…_deferred Signed-off-by: tyxia <tyxia@google.com>
/wait-any |
/wait-any |
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.
Approach LGTM, will defer to @yanavlasov on the detailed review, thanks!
Signed-off-by: tyxia <tyxia@google.com>
Signed-off-by: tyxia <tyxia@google.com>
Thanks all for review! @yanavlasov PTAL, Thanks |
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 with minor comments.
/wait
Signed-off-by: tyxia <tyxia@google.com>
…nvoyproxy#34491) --------- Signed-off-by: tyxia <tyxia@google.com> Signed-off-by: Fernando Cainelli <fernando.cainelli-external@getyourguide.com>
…nvoyproxy#34491) --------- Signed-off-by: tyxia <tyxia@google.com> Signed-off-by: Fernando Cainelli <fernando.cainelli-external@getyourguide.com>
…nvoyproxy#34491) --------- Signed-off-by: tyxia <tyxia@google.com> Signed-off-by: Fernando Cainelli <fernando.cainelli-external@getyourguide.com>
…nvoyproxy#34491) --------- Signed-off-by: tyxia <tyxia@google.com> Signed-off-by: Fernando Cainelli <fernando.cainelli-external@getyourguide.com>
Below are two key parts of this PR, they are added together because observability mode is the only use case (thus test case) of deferred stream closure (at the moment).
(1) Deferred stream closure (Part 2):
(2) Observability mode:
Testing: Integration test + unit test.