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

etcdserver/api/etcdhttp: log successful etcd server side health check in debug level #12677

Conversation

chaochn47
Copy link
Member

When we have an external component that checks /health periodically, the
etcd server logs can be quite verbose (e.g., DDOS-ing against insure
etcd health check can lead to disk space full due to large log files).

This change was introduced in #11704.

While we keep the warning logs for etcd health check failures, the
success (or OK) log level should be set to DEBUG.

Fixes #12676

… in debug level

When we have an external component that checks /health periodically, the
etcd server logs can be quite verbose (e.g., DDOS-ing against insure
etcd health check can lead to disk space full due to large log files).

This change was introduced in etcd-io#11704.

While we keep the warning logs for etcd health check failures, the
success (or OK) log level should be set to DEBUG.

Fixes etcd-io#12676
@chaochn47
Copy link
Member Author

@gyuho

@gyuho
Copy link
Contributor

gyuho commented Feb 9, 2021

/cc @ptabor

CHANGELOG-3.5.md Outdated
@@ -144,6 +144,7 @@ Note that any `etcd_debugging_*` metrics are experimental and subject to change.
- [Fix server panic](https://github.com/etcd-io/etcd/pull/12288) when force-new-cluster flag is enabled in a cluster which had learner node.
- Add [`--self-signed-cert-validity`](https://github.com/etcd-io/etcd/pull/12429) flag to support setting certificate expiration time.
- Notice, certificates generated by etcd are valid for 1 year by default when specifying the auto-tls or peer-auto-tls option.
- Log [successful etcd server-side health check in debug level](https://github.com/etcd-io/etcd/pull/12677).
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this below line 117?

Below

Copy link
Member Author

Choose a reason for hiding this comment

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

ack. moved this CHANGELOG entry to line 118.

@codecov-io
Copy link

Codecov Report

Merging #12677 (9cef412) into master (96a9860) will decrease coverage by 22.41%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #12677       +/-   ##
===========================================
- Coverage   68.70%   46.29%   -22.42%     
===========================================
  Files         410      407        -3     
  Lines       32825    32804       -21     
===========================================
- Hits        22553    15185     -7368     
- Misses       8287    15657     +7370     
+ Partials     1985     1962       -23     
Flag Coverage Δ
all 46.29% <100.00%> (-22.42%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
server/etcdserver/api/etcdhttp/metrics.go 70.42% <100.00%> (ø)
pkg/types/slice.go 0.00% <0.00%> (-100.00%) ⬇️
pkg/ioutil/reader.go 0.00% <0.00%> (-100.00%) ⬇️
pkg/ioutil/readcloser.go 0.00% <0.00%> (-100.00%) ⬇️
client/v2/cluster_error.go 0.00% <0.00%> (-100.00%) ⬇️
client/v3/ordering/util.go 0.00% <0.00%> (-100.00%) ⬇️
pkg/tlsutil/cipher_suites.go 0.00% <0.00%> (-100.00%) ⬇️
client/v3/naming/endpoints/endpoints.go 0.00% <0.00%> (-100.00%) ⬇️
client/v3/leasing/util.go 0.00% <0.00%> (-98.04%) ⬇️
pkg/report/report.go 0.00% <0.00%> (-95.58%) ⬇️
... and 246 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96a9860...9cef412. Read the comment docs.

Copy link
Member

@spzala spzala left a comment

Choose a reason for hiding this comment

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

lgtm after @gyuho 's comment is addressed.
Thanks @chaochn47

@chaochn47 chaochn47 force-pushed the reduce_verbosity_of_etcd_server_side_success_health_check_logs branch from 9cef412 to 028d351 Compare February 9, 2021 03:47
@gyuho
Copy link
Contributor

gyuho commented Feb 9, 2021

@chaochn47 Can we backport this to release-3.4 branch? Thanks!

@gyuho gyuho merged commit 44c889a into etcd-io:master Feb 9, 2021
chaochn47 added a commit to chaochn47/etcd that referenced this pull request Feb 9, 2021
@chaochn47 chaochn47 deleted the reduce_verbosity_of_etcd_server_side_success_health_check_logs branch February 9, 2021 05:51
hexfusion pushed a commit to hexfusion/etcd that referenced this pull request Jun 16, 2021
tjungblu pushed a commit to tjungblu/etcd that referenced this pull request Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Reduce verbosity of etcd server-side health check logs
4 participants