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

access logging: introduce critical ALS endpoint #17486

Closed
wants to merge 47 commits into from

Conversation

Shikugawa
Copy link
Member

Signed-off-by: Shikugawa rei@tetrate.io

Commit Message: Add an ALS endpoint to ensure that the message reaches the destination. This endpoint is designed to return an ACK/NACK as a response, and if a NACK is received, Envoy will buffer the message and take steps to resend it. The log to be buffered is determined by the AccessLogFilter.
Additional Description:
Risk Level: Low
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

cc @lizan

Signed-off-by: Shikugawa <rei@tetrate.io>
@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: #17486 was opened by Shikugawa.

see: more, trace.

@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @htuch
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #17486 was opened by Shikugawa.

see: more, trace.

@mattklein123
Copy link
Member

cc @snowp can you also take a look at this? This is similar to some of the things we are doing at Lyft and we should see if there are any useful overlaps.

@snowp
Copy link
Contributor

snowp commented Jul 26, 2021

This is super cool, I think this kind of pattern (bidi stream that sends payloads, buffers until ack) would be useful in other places as well. Do you think it would make sense to create a generic BufferingAsyncClient or something similar that allows hooking this functionality up to arbitrary bidi streams?

I think for example this could be employed by the gRPC stats sink, which currently would suffer the same kind of loss that the ALS sink does.

Signed-off-by: Shikugawa <rei@tetrate.io>
Signed-off-by: Shikugawa <rei@tetrate.io>
Signed-off-by: Shikugawa <rei@tetrate.io>
Signed-off-by: Shikugawa <rei@tetrate.io>
Signed-off-by: Shikugawa <rei@tetrate.io>
@Shikugawa Shikugawa marked this pull request as ready for review August 2, 2021 11:43
@Shikugawa
Copy link
Member Author

@snowp @mattklein123 @lizan Finished to implement codes (except docs). Could you take a look please?

Signed-off-by: Shikugawa <rei@tetrate.io>
Signed-off-by: Shikugawa <rei@tetrate.io>
Signed-off-by: Shikugawa <rei@tetrate.io>
Signed-off-by: Shikugawa <rei@tetrate.io>
Signed-off-by: Shikugawa <rei@tetrate.io>
Signed-off-by: Shikugawa <rei@tetrate.io>
Signed-off-by: Shikugawa <rei@tetrate.io>
Signed-off-by: Shikugawa <rei@tetrate.io>
@Shikugawa
Copy link
Member Author

@snowp @htuch Could you take a look?

Signed-off-by: Shikugawa <rei@tetrate.io>
Signed-off-by: Shikugawa <rei@tetrate.io>
Signed-off-by: Shikugawa <rei@tetrate.io>
Signed-off-by: Shikugawa <rei@tetrate.io>
@Shikugawa
Copy link
Member Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17486 (comment) was created by @Shikugawa.

see: more, trace.

@mattklein123
Copy link
Member

Needs a main merge. cc @snowp to help finish the review on this.

/wait

@github-actions
Copy link

github-actions bot commented Nov 5, 2021

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Nov 5, 2021
@Shikugawa Shikugawa removed the stale stalebot believes this issue/PR has not been touched recently label Nov 6, 2021
lizan pushed a commit that referenced this pull request Nov 10, 2021
…8129)

Commit Message: grpc: implement BufferedAsyncClient for bidirectional gRPC stream
Additional Description: This function is a component for buffering and tracking the state of messages sent in the gRPC bidirectional stream. This component makes it possible to monitor and control the transmission status of highly granular messages from the Envoy side, and is identical to the one implemented in #17486.
Risk Level: Low
Testing: Unit
Docs Changes:
Release Notes:
Platform Specific Features:

Signed-off-by: Shikugawa <rei@tetrate.io>
Signed-off-by: Shikugawa <rei@tetrate.io>
@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @htuch
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #17486 was synchronize by Shikugawa.

see: more, trace.

Signed-off-by: Shikugawa <rei@tetrate.io>
Signed-off-by: Shikugawa <rei@tetrate.io>
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.

/wait


// Size limit (in bytes) of the buffer used to store messages during processing in a
// critical logger. A critical logger buffers messages until it receives an ACK from upstream.
// The default is 16384.
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens when the buffer size is exceeded? Can you clarify in the comment, please?

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Dec 12, 2021
@Shikugawa Shikugawa removed the stale stalebot believes this issue/PR has not been touched recently label Dec 15, 2021
@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Jan 14, 2022
@github-actions
Copy link

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot closed this Jan 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api stale stalebot believes this issue/PR has not been touched recently waiting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants