-
Notifications
You must be signed in to change notification settings - Fork 2.2k
compact: Add resolution information to compaction and downsample logs #8353
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
base: main
Are you sure you want to change the base?
compact: Add resolution information to compaction and downsample logs #8353
Conversation
Signed-off-by: Naman-B-Parlecha <[email protected]>
7363dc6
to
64f49dd
Compare
@MichaHoffmann @GiedriusS Any specific file that i m missing to add it? |
It will be very helpful, thanks. In addition I think it would be nice to have a log at debug level in https://github.com/thanos-io/thanos/blob/main/pkg/compact/downsample/downsample.go |
@julienlau refactoring |
Rather than having to add resolution on all logs, would it be good if Line 368 in ddd5ff8
resolution = raw resolution = 5m resolution = 1hr or having it seperated in each log needed??
|
@Naman-B-Parlecha We can have it just in the logger. I would just use 0s, 5m and 1h as resolution value, which is the standard duration string in Go. |
Yes works |
Signed-off-by: Naman-B-Parlecha <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Just small nits
pkg/compact/compact.go
Outdated
resolutionLabel := m.Thanos.ResolutionString() | ||
group, err = NewGroup( | ||
log.With(g.logger, "group", fmt.Sprintf("%s@%v", resolutionLabel, lbls.String()), "groupKey", groupKey), | ||
log.With(g.logger, "group", fmt.Sprintf("%s@%v", metadata.ResolutionToString(resolutionVal), lbls.String()), "groupKey", groupKey), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we change the existing group
field? If we want to be explicit I would add another logger field for resolution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i was thinking if '0' stands for the resolution anyways in logs, we can just have it logs in better way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added a new field called resolution
but ya having this setup would be better as it already exists
Signed-off-by: Naman-B-Parlecha <[email protected]>
Signed-off-by: Naman-B-Parlecha <[email protected]>
@yeya24 PTAL added Compact Level on @julienlau 's request. |
@yeya24 PTAL :D |
Signed-off-by: Naman-B-Parlecha <[email protected]>
Signed-off-by: Naman-B-Parlecha <[email protected]>
ac51770
to
bfd729c
Compare
weird ci |
Changes
Checks #8350
Adding resolution level (raw, 5m, 1h) to compact and downsample log messages to have more context
Verification