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.

Sign up for GitHub

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

Jump to bottom

ext_proc: observability mode with deferred stream closure (part 2) #34491

Merged
merged 24 commits into from
Jun 27, 2024
Merged

ext_proc: observability mode with deferred stream closure (part 2) #34491

merged 24 commits into from
Jun 27, 2024

Conversation

tyxia
Copy link
Member

@tyxia tyxia commented Jun 3, 2024 •
edited
Loading

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:

  • Implement the observability mode on all phase of Http request and response life cycle
  • TODO: Next PR will add the logging/billing support

Testing: Integration test + unit test.

@repokitteh-read-only RepoKitteh - Read Only
Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #34491 was opened by tyxia.

see: more, trace.

tyxia added 14 commits June 3, 2024 19:51
@tyxia
fix format
594e088 
Signed-off-by: tyxia <tyxia@google.com>
@tyxia
fix format
d55b082 
Signed-off-by: tyxia <tyxia@google.com>
@tyxia
fix
c8d21b0 
Signed-off-by: tyxia <tyxia@google.com>
@tyxia
fix
37adc18 
Signed-off-by: tyxia <tyxia@google.com>
@tyxia
add more test
6d497f0 
Signed-off-by: tyxia <tyxia@google.com>
@tyxia
add more test
7023a85 
Signed-off-by: tyxia <tyxia@google.com>
@tyxia
Merge branch 'main' of https://github.com/envoyproxy/envoy into async…
c752d91 
…_deferred

Signed-off-by: tyxia <tyxia@google.com>
@tyxia
tweaks
728622e 
Signed-off-by: tyxia <tyxia@google.com>
@tyxia
Merge branch 'main' of https://github.com/envoyproxy/envoy into async…
14627cb 
…_deferred

Signed-off-by: tyxia <tyxia@google.com>
@tyxia
simulate time
f964c1d 
Signed-off-by: tyxia <tyxia@google.com>
@tyxia
fix format
0c5fe19 
Signed-off-by: tyxia <tyxia@google.com>
@tyxia
Merge branch 'main' of https://github.com/envoyproxy/envoy into async…
c3f074a 
…_deferred

Signed-off-by: tyxia <tyxia@google.com>
@tyxia
fix
412023e 
Signed-off-by: tyxia <tyxia@google.com>
@tyxia
fix
bab6bcc 
Signed-off-by: tyxia <tyxia@google.com>
@tyxia tyxia marked this pull request as ready for review June 6, 2024 00:25
@tyxia
Copy link
Member Author

tyxia commented Jun 6, 2024

/assign @htuch @yanavlasov

PTAL, Thanks!

@htuch htuch changed the title ext_proc: deferred stream closure (part 2) + observability mode ext_proc: observability mode with deferred stream closure (part 2) Jun 6, 2024
@htuch
Copy link
Member

htuch commented Jun 6, 2024

I modified the PR title to make it clearer this is primarily about o11y mode.

htuch
htuch reviewed Jun 6, 2024
View reviewed changes
Copy link
Member

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

tyxia added 3 commits June 7, 2024 00:14
@tyxia
add more unit tests
2900510 
Signed-off-by: tyxia <tyxia@google.com>
@tyxia
add more comments
ff29fc7 
Signed-off-by: tyxia <tyxia@google.com>
@tyxia
release note
31367e2 
Signed-off-by: tyxia <tyxia@google.com>
@alyssawilk
Copy link
Contributor

@yanavlasov PTAL (or swap reviewers)

@tyxia
Merge branch 'main' of https://github.com/envoyproxy/envoy into async…
a001a0b 
…_deferred

Signed-off-by: tyxia <tyxia@google.com>
@yanavlasov
Copy link
Contributor

/wait-any

@tyxia
Copy link
Member Author

tyxia commented Jun 18, 2024

/wait-any

htuch
htuch reviewed Jun 19, 2024
View reviewed changes
Copy link
Member

@htuch htuch left a 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!

source/extensions/filters/http/ext_proc/ext_proc.cc Outdated Show resolved Hide resolved
source/extensions/filters/http/ext_proc/ext_proc.cc Outdated Show resolved Hide resolved
source/extensions/filters/http/ext_proc/client_impl.cc Show resolved Hide resolved
@tyxia
add test for onRemoteClose
a1447a9 
Signed-off-by: tyxia <tyxia@google.com>
@tyxia
utility function
278bbae 
Signed-off-by: tyxia <tyxia@google.com>
@tyxia
stream id
a7dce46 
Signed-off-by: tyxia <tyxia@google.com>
@tyxia
Copy link
Member Author

tyxia commented Jun 21, 2024

Thanks all for review!

@yanavlasov PTAL, Thanks

@tyxia tyxia requested a review from yanavlasov June 21, 2024 13:03
yanavlasov
yanavlasov reviewed Jun 25, 2024
View reviewed changes
Copy link
Contributor

@yanavlasov yanavlasov left a 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

changelogs/current.yaml Outdated Show resolved Hide resolved
source/extensions/filters/http/ext_proc/ext_proc.cc Show resolved Hide resolved
@tyxia
update per comment
a960f5c 
Signed-off-by: tyxia <tyxia@google.com>
@tyxia tyxia requested a review from yanavlasov June 26, 2024 21:29
@yanavlasov yanavlasov merged commit 1a98fd5 into envoyproxy:main Jun 27, 2024
50 of 51 checks passed
@tyxia tyxia deleted the async_deferred branch June 27, 2024 15:18
@tyxia tyxia mentioned this pull request Jul 1, 2024
Closed
cainelli pushed a commit to cainelli/envoy that referenced this pull request Jul 5, 2024
@tyxia @cainelli
ext_proc: observability mode with deferred stream closure (part 2) (e…
9e3be9a 
…nvoyproxy#34491)

---------

Signed-off-by: tyxia <tyxia@google.com>
Signed-off-by: Fernando Cainelli <fernando.cainelli-external@getyourguide.com>
cainelli pushed a commit to cainelli/envoy that referenced this pull request Jul 5, 2024
@tyxia @cainelli
ext_proc: observability mode with deferred stream closure (part 2) (e…
fd68e2b 
…nvoyproxy#34491)

---------

Signed-off-by: tyxia <tyxia@google.com>
Signed-off-by: Fernando Cainelli <fernando.cainelli-external@getyourguide.com>
cainelli pushed a commit to cainelli/envoy that referenced this pull request Jul 5, 2024
@tyxia @cainelli
ext_proc: observability mode with deferred stream closure (part 2) (e…
60eb709 
…nvoyproxy#34491)

---------

Signed-off-by: tyxia <tyxia@google.com>
Signed-off-by: Fernando Cainelli <fernando.cainelli-external@getyourguide.com>
cainelli pushed a commit to cainelli/envoy that referenced this pull request Jul 5, 2024
@tyxia @cainelli
ext_proc: observability mode with deferred stream closure (part 2) (e…
084c3f9 
…nvoyproxy#34491)

---------

Signed-off-by: tyxia <tyxia@google.com>
Signed-off-by: Fernando Cainelli <fernando.cainelli-external@getyourguide.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@htuch htuch htuch left review comments

@yanavlasov yanavlasov yanavlasov approved these changes

@mattklein123 mattklein123 Awaiting requested review from mattklein123 mattklein123 is a code owner

Assignees

@yanavlasov yanavlasov

@htuch htuch

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@tyxia @htuch @alyssawilk @yanavlasov