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

hc: add health event sink interface and file sink implementation #27419

Merged
merged 22 commits into from
May 25, 2023
Merged

hc: add health event sink interface and file sink implementation #27419

merged 22 commits into from
May 25, 2023

Conversation

botengyao
Copy link
Member

@botengyao botengyao commented May 16, 2023 •
edited
Loading

Added health event sink extension, and provided the file sink implementation to deprecate the event_log_path field.

Commit Message:
Additional Description:
Risk Level: Low
Testing: unit test
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

@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: #27419 was opened by botengyao.

see: more, trace.

@botengyao botengyao changed the title [health check] Add access log to health check [HC] Add access log to health check May 16, 2023
@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 @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #27419 was opened by botengyao.

see: more, trace.

@botengyao botengyao changed the title [HC] Add access log to health check HC: Add access log to health check May 16, 2023
botengyao added 2 commits May 16, 2023 19:29
@botengyao
change comments
47a5da8 
Signed-off-by: Boteng Yao <[email protected]>
@botengyao
fix build errors
1152709 
Signed-off-by: Boteng Yao <[email protected]>
@botengyao botengyao changed the title HC: Add access log to health check hc: add access log to health check May 16, 2023
@botengyao
format fix
796ebe2 
Signed-off-by: Boteng Yao <[email protected]>
@botengyao botengyao changed the title hc: add access log to health check hc: add access log for health check event May 17, 2023
@botengyao
build error
a15c593 
Signed-off-by: Boteng Yao <[email protected]>
@botengyao botengyao marked this pull request as ready for review May 17, 2023 14:00
@botengyao
Copy link
Member Author

/assign @yanavlasov

yanavlasov
yanavlasov requested changes May 17, 2023
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.

/wait

source/common/upstream/health_checker_event_logger.h Outdated
const auto& validator_config =
Envoy::MessageUtil::anyConvertAndValidate<envoy::config::accesslog::v3::AccessLog>(
config.typed_config(), context.messageValidationVisitor());
access_logs_.emplace_back(AccessLog::AccessLogFactory::fromProto(validator_config, context));
Copy link
Contributor

Choose a reason for hiding this comment

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

After looking at it more, I do not think we should reuse the request access log here, since we loose ability to log the HealthCheckEvent proto without having to convert it to JSON first.

I think it would be cleaner to create a new interface HealthCheckEventSink with a single log method that accepts the HealthCheckEvent protobuf as a parameter.

A custom implementation of this sink can do anything it wants with the protobuf (including logging into the request access log if need be).

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. Right now we don't need to convert to json to log it, but just put the proto to stream_info. Right, I think a new interface is cleaner.

api/envoy/config/core/v3/health_check.proto Show resolved Hide resolved
@botengyao
clean up
5c94600 
Signed-off-by: Boteng Yao <[email protected]>
@botengyao botengyao changed the title hc: add access log for health check event hc: add health event sink interface for health check May 23, 2023
@botengyao
remove ad-hoc test logs
3b3437e 
Signed-off-by: Boteng Yao <[email protected]>
yanavlasov
yanavlasov requested changes May 23, 2023
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.

/wait

source/common/upstream/health_checker_event_logger.h Outdated Show resolved Hide resolved
source/common/upstream/health_checker_event_logger.h Outdated Show resolved Hide resolved
@botengyao
fix toctree error
7e7a09b 
Signed-off-by: Boteng Yao <[email protected]>
@botengyao botengyao changed the title hc: add health event sink interface for health check hc: add health event sink interface and file sink implementation May 23, 2023
botengyao added 2 commits May 23, 2023 21:00
@botengyao
update fuzz corpus
59ae8de 
Signed-off-by: Boteng Yao <[email protected]>
@botengyao
fix clang-tidy
ada26f6 
Signed-off-by: Boteng Yao <[email protected]>
yanavlasov
yanavlasov requested changes May 24, 2023
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 small nit to add TODO.

/wait

source/common/upstream/health_checker_event_logger.h Show resolved Hide resolved
adisuissa
adisuissa reviewed May 24, 2023
View reviewed changes
Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

Thanks!
Left a few small nits but overall LGTM.

api/envoy/config/core/v3/health_check.proto Outdated Show resolved Hide resolved
envoy/server/health_checker_config.h Show resolved Hide resolved
source/common/upstream/health_checker_impl.cc Outdated Show resolved Hide resolved
source/common/upstream/health_checker_event_logger.h Outdated Show resolved Hide resolved
botengyao added 4 commits May 24, 2023 18:08
@botengyao
rename catagory and fixed nits
ea23450 
Signed-off-by: Boteng Yao <[email protected]>
@botengyao
fix format
1efce6f 
Signed-off-by: Boteng Yao <[email protected]>
@botengyao
modify comments
a28e496 
Signed-off-by: Boteng Yao <[email protected]>
@botengyao
add change logs
41b3f32 
Signed-off-by: Boteng Yao <[email protected]>
@botengyao
Copy link
Member Author

/retest

@yanavlasov yanavlasov merged commit 881bc03 into envoyproxy:main May 25, 2023
reskin89 pushed a commit to reskin89/envoy that referenced this pull request Jul 11, 2023
@botengyao @reskin89
hc: add health event sink interface and file sink implementation (env…
5f88efc 
…oyproxy#27419)

* add access log to health check

Signed-off-by: Boteng Yao <[email protected]>
Signed-off-by: Ryan Eskin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adisuissa adisuissa adisuissa left review comments

@yanavlasov yanavlasov yanavlasov approved these changes

@zuercher zuercher Awaiting requested review from zuercher

@mattklein123 mattklein123 Awaiting requested review from mattklein123

Assignees

@adisuissa adisuissa

@yanavlasov yanavlasov

Labels
api
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@botengyao @adisuissa @yanavlasov