CBG-5109 Fix & vet log *fCtx function arguments

[torcolvin]

CBG-5109 Fix & vet log *fCtx function arguments

As per the documentation in https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/printf:

To enable printf checking on a function that is not found by this analyzer's heuristics (for example, because control is obscured by dynamic method calls), insert a bogus call:

if false {
	_ = fmt.Sprintf(format, args...) // enable printf checking
}
...

}```

• add bogus calls to allow go vet (and hence golangci-lint) to catch errors.
• Fix type errors in logger functions and require a constant format string.

[Copilot]

Pull request overview

This PR enhances logging function safety by enabling Go's printf-style format string verification through the go vet tool. It adds no-op calls to fmt.Sprintf within logging functions (per the golang.org/x/tools/go/analysis/passes/printf recommendation) and corrects existing format string mismatches found throughout the codebase.

Changes:

• Added bogus fmt.Sprintf calls in logging functions to enable compile-time format string verification
• Fixed incorrect format specifiers and missing format strings in logging calls across the codebase
• Renamed parameter from fmt to format in RedactErrorf for clarity

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
base/logging.go	Added printf checking to all *Ctx logging functions
base/redactable_error.go	Added printf checking to RedactErrorf and renamed parameter
base/main_test_util.go	Added printf checking to test utility functions
base/main_test_bucket_pool_util.go	Added printf checking to TestBucketPool logging methods
base/logging_config.go	Fixed missing format string in error message logging
base/devmode_on.go	Added printf checking to panic recovery function
base/dcp_feed_type.go	Fixed missing format arguments in InfofCtx call
base/dcp_dest.go	Fixed incorrect format specifier for partition parameter
base/collection_n1ql_common.go	Added printf checking to SlowQueryLog function
rest/server_context_test.go	Fixed missing format strings in test logging calls
rest/server_context.go	Fixed format strings for warning and info messages
rest/oidc_api_test.go	Fixed missing format string in error logging
rest/doc_api.go	Removed unnecessary fmt.Sprintf wrapper in log call
rest/config_manager.go	Fixed missing format strings in debug and info logs
db/sg_replicate_conflict_resolver.go	Fixed missing format string in warning message
db/sg_replicate_cfg.go	Fixed missing replicationID argument in warning messages
db/database.go	Fixed missing format strings in info messages
db/change_cache.go	Fixed incomplete format strings in warning messages
db/blip_handler_collections.go	Fixed missing format string in warning message
db/background_mgr_attachment_migration.go	Fixed missing format strings in info messages
db/attachment_compaction.go	Fixed incorrect format specifier for CAS value
db/active_replicator_checkpointer.go	Removed unused error argument from log message
base/main_test_bucket_pool.go	Fixed incorrect format verb for error value

[Copilot]

The error message indicates a failure to retrieve an existing checkpoint but doesn't mention the error that occurred. Consider adding the error details to help with debugging: \"Revision mismatch in setCheckpoint, and unable to retrieve existing (%v), will retry\", getErr.

Suggested change

	base.InfofCtx(c.ctx, base.KeyReplicate, "Revision mismatch in setCheckpoint, and unable to retrieve existing, will retry")
	base.InfofCtx(c.ctx, base.KeyReplicate, "Revision mismatch in setCheckpoint, and unable to retrieve existing (%v), will retry", getErr)

[bbrks]

Nice that it found all of these legitimate formatting issues - completely gross way of having to force the check :(

I think we can minimise the impact by just adding the noop to the lowest level of the logging callstack?

[bbrks]

Forcing the check just once here in logTo allows basically all of the other logging functions to still be checked - there's no need to define this in every place.

It greatly reduces the impact of the noise of enabling this check (which I think is extremely valuable)

There are still some exceptions, like the RedactableError and test logger - but otherwise this looks good.

[torcolvin]

Does this work? It didn't work for me because I thought it only checks one layer of functions?

I would be happy if you were to play with this (but not too long) because maybe I just missed something.

go vet plus -printfuncs argument which didn't seem to work for this. I did try to add logging to a copy of golang.org/x/tools/go/analysis/passes/printf but I wasn't able to determine why it couldn't detect the functions.

I did wonder if renaming the functions InfoCtxf or just Infof would fix the problem but that would make backports awful. Though that would align with the naming conventions.

[bbrks]

I tried it locally with just go vet ./base and it worked. I couldn't push it any further down though. Idk if there's a limit on how far it checks?

[torcolvin]

Thanks! I really thought I verified this before but it seems to work fine like this.

Adding to TestBucketPool.Fatalf is required because of the addPrefixes call.

[torcolvin]

Do you think this PR is worth backporting once we sort out the details?

[bbrks]

Yes I think so - at the very least the actual fixes to the wrong formatting directives