edac: Fix printf-style formatting in slog call - #3759
Open
at-blacknight wants to merge 1 commit into
Open
Conversation
The Debug call added in prometheus#3734 passes printf verbs and positional arguments to slog, which does not format them. The format string is logged verbatim and the arguments are consumed as key/value pairs, so controller and channel numbers become attribute keys: a duplicate key when the two are equal, and otherwise a key whose name changes on every iteration. Under --log.format=json that is an object with a repeated key, and parsers silently drop one of the values. The csrow number and the error, the two things worth reading, are the only parts that survive, and they appear as values of meaningless keys. %w compounds it: that is an fmt.Errorf directive and not a valid Printf verb, left behind when the error return this replaced was converted to a log line. Pass the values as named attributes instead. Metric output is unchanged. Signed-off-by: Adam Butler <github@at-blacknight.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
@SuperQ — small follow-up to #3734.
That PR converted the
ch*_ue_counterror return into a debug log, and thefmt.Errorfformat string came across with it.slogdoes no formatting, so the verbs are logged literally and the arguments after the message are consumed as key/value pairs:controllerNumberbecame a key withcsrowNumberas its value, andchannelNumberbecame a second key with the error as its value, so the controller/csrow/channel the message meant to report are no longer readable and the record carries0twice. Where the controller and channel numbers differ the second key is named after the channel instead, so the key set changes from one iteration to the next. Under--log.format=jsonthat is an object with a repeated key, and parsers silently drop one of the values.%wcompounds it: that is anfmt.Errorfdirective, not a validPrintfverb.After:
Reproduces on the existing fixtures:
mc0/csrow2hasch0_ce_countbut noch0_ue_count, so running with--log.level=debugagainstcollector/fixtures/syshits the branch. Present in v1.12.1 and on master. Metric output is unchanged — 20node_edacseries before and after, identical — andgofmt,go vetandend-to-end-test.share clean.On why CI didn't catch it, in case it's useful:
go vet's slog analyzer only fires when a key-position argument isn't a string or the arguments don't pair up, and here the count is even and all three are strings, so the call looks well formed to it. The e2e test does execute this line with debug logging enabled, but only diffs the metrics output. I have a smallgo/asttest that rejects Printf verbs in slog messages across the collector package — happy to raise it separately if you'd want it, equally happy to leave it.