feat(promslog): implement reserved keys, rename duplicates #746
Conversation
This file contains 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
This commit adds support for "reserved" keys, where upon detecting a duplicate log attribute key, it will rename the key to prevent collisions/issues with core slog attribute keys in use. The reserved keys are: - `level` - `source` (for standard slog style) - `caller` (for legacy go-kit style) - `time` (for standard slog style) - `ts` (for legacy go-kit style) By supporting reserved keys, we allow users of the library to use these keys for their own supplemental log attributes. Fixes: prometheus#745 Example test output: ``` === RUN TestReservedKeys/slog_log_style time=2025-01-13T18:32:46.508Z level=INFO source=slog_test.go:212 msg="reserved keys test for slog_log_style" logged_level="surprise! I'm a string" logged_source="surprise! I'm a string" logged_time="surprise! I'm a string" === RUN TestReservedKeys/go-kit_log_style ts=2025-01-13T18:32:46.508Z level=info caller=slog_test.go:212 msg="reserved keys test for go-kit_log_style" logged_level="surprise! I'm a string" logged_caller="surprise! I'm a string" logged_ts="surprise! I'm a string" ``` Note: this implementation only technically removes duplicates of our core reserved keys. If a user adds multiple instances of a reserved key, the rest get renamed to `logged_$key`, which means there could be duplicate attributes with `logged_$key`. This is mostly to simplify implementation so we don't need to bother reference counting in the replace attr function to do things like `logged_$key1`, `logged_$key2`, etc. Signed-off-by: TJ Hoplock <[email protected]>
|
Huh, I didn't get a lint error locally. Looking |
Signed-off-by: TJ Hoplock <[email protected]>
|
🚀 |
bwplotka
reviewed
Jan 17, 2025
| require.Containsf(t, output, fmt.Sprintf("%s%s=\"%s\"", reservedKeyPrefix, tc.timeKey, reservedKeyTestVal), "Expected duplicate time key to be renamed") | ||
|
|
||
| // Print logs for humans to see, if needed. | ||
| fmt.Println(buf.String()) |
There was a problem hiding this comment.
You could use t.Log too, but not a blocker.
|
This feels like a bug fix to me, even if the behavior changes slightly. What do you think @bwplotka? |
|
Yea, it feels like a bug, but also it was always there with the new |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This commit adds support for "reserved" keys, where upon detecting a
duplicate log attribute key, it will rename the key to prevent
collisions/issues with core slog attribute keys in use.
The reserved keys are:
levelsource(for standard slog style)caller(for legacy go-kit style)time(for standard slog style)ts(for legacy go-kit style)By supporting reserved keys, we allow users of the library to use these
keys for their own supplemental log attributes.
Fixes: #745
Example test output:
Note: this implementation only technically removes duplicates of our
core reserved keys. If a user adds multiple instances of a reserved key,
the rest get renamed to
logged_$key, which means there could beduplicate attributes with
logged_$key. This is mostly to simplifyimplementation so we don't need to bother reference counting in the
replace attr function to do things like
logged_$key1,logged_$key2,etc.
Signed-off-by: TJ Hoplock [email protected]