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

log: bump logrotate dep, switch to zstd compressor #2238

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jharveyb
Copy link
Contributor

Replacement for #2237 since logrotate was updated.

This issue lightninglabs/taproot-assets#1084 prompted me to look at how we could switch from gzip to zstd for log compression (better compression at the default level: https://pkg.go.dev/github.com/klauspost/compress/zstd#readme-performance).

The klauspost ZSTD implementation seems like the best pure-Go choice, at least until it gets into the stdlib.

@jharveyb
Copy link
Contributor Author

Needed to pick an older version of compress as they stopped testing builds with go1.17 at version v1.15.15.

klauspost/compress@0793ca1

@coveralls
Copy link

coveralls commented Aug 16, 2024

Pull Request Test Coverage Report for Build 10568577728

Details

  • 0 of 23 (0.0%) changed or added relevant lines in 2 files are covered.
  • 11 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.04%) to 57.192%

Changes Missing Coverage Covered Lines Changed/Added Lines %
config.go 0 10 0.0%
log.go 0 13 0.0%
Files with Coverage Reduction New Missed Lines %
connmgr/connmanager.go 1 86.27%
log.go 1 19.4%
peer/peer.go 9 73.49%
Totals Coverage Status
Change from base Build 10555907676: -0.04%
Covered Lines: 29836
Relevant Lines: 52168

💛 - Coveralls

@jrick
Copy link
Member

jrick commented Aug 16, 2024

v1.1.1 fixes an (albeit unlikely) data race.

Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

LTM after a version bump to v1.1. as mentioned above.

@jharveyb
Copy link
Contributor Author

Will update this to be a config option that defaults to the (existing) gzip before merge.

@jharveyb
Copy link
Contributor Author

Added a config option that allows gzip and zstd as options. Tested as follows, generating logs by starting testnet IBD with debuglevel=trace:

  • An invalid compressor string, 'nocompress' is rejected both as CLI flag and config file option
  • The help message shows the new flag
  • Setting logcompressor=zstd via config file and CLI worked, and produced compressed logs with suffix zst that zgrep handled successfully
  • Setting logcompressor=gzip via config file worked, and produced compressed logs with suffix gz that zgrep handled successfully
  • Omitting the logcompressor option from the config file led to compressed logfiles with suffiz gz, maintaining the existing behavior

@guggero guggero self-requested a review August 27, 2024 09:05
Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

utACK, LGTM 🎉

config.go Outdated Show resolved Hide resolved
@jharveyb
Copy link
Contributor Author

Updated to address feedback.

doc.go Outdated Show resolved Hide resolved
log.go Outdated Show resolved Hide resolved
@jharveyb jharveyb force-pushed the log_compress_zstd branch 3 times, most recently from 71baf98 to 99db92b Compare September 12, 2024 16:35
Copy link
Collaborator

@yyforyongyu yyforyongyu left a comment

Choose a reason for hiding this comment

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

LGTM!

@jharveyb
Copy link
Contributor Author

jharveyb commented Oct 7, 2024

Rebased.

@yyforyongyu
Copy link
Collaborator

hmmm weird CI failures...

@kcalvinalvin
Copy link
Collaborator

Oh sorry I think this is me forgetting to put in build tags for getchaintips_test.go and invalidate_reconsider_block_test.go. CI passes for me on my branch https://github.com/kcalvinalvin/btcd/actions/runs/11233768822/job/31228158339

diff --git a/integration/getchaintips_test.go b/integration/getchaintips_test.go
index 1570ba74..5a4d39b5 100644
--- a/integration/getchaintips_test.go
+++ b/integration/getchaintips_test.go
@@ -1,3 +1,7 @@
+// This file is ignored during the regular tests due to the following build tag.
+//go:build rpctest
+// +build rpctest
+
 package integration
 
 import (
diff --git a/integration/invalidate_reconsider_block_test.go b/integration/invalidate_reconsider_block_test.go
index 4fe6ff00..cfc59840 100644
--- a/integration/invalidate_reconsider_block_test.go
+++ b/integration/invalidate_reconsider_block_test.go
@@ -1,3 +1,7 @@
+// This file is ignored during the regular tests due to the following build tag.
+//go:build rpctest
+// +build rpctest
+
 package integration

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

FWIW, this ends up adding an entirely new dependency, just for slightly better log compression, how necessary is this? Seems like a nice to have, since a user can just watch the logs on disk, then rewrite them with their desired compression format.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants