Upgrade to latest mimir-prometheus@main #9844
Conversation
aknuds1
force-pushed
the
arve/upgrade-mimir-prometheus
branch
22 times, most recently
from
cb6d823
to
a1b4f48
Compare
aknuds1
changed the title
aknuds1
force-pushed
the
arve/upgrade-mimir-prometheus
branch
from
a1b4f48
to
6a3bfe1
Compare
|
To address one small set of test failures: #9854 |
aknuds1
force-pushed
the
arve/upgrade-mimir-prometheus
branch
5 times, most recently
from
71feed6
to
69c0bd2
Compare
| type goKitHandler struct { | ||
| logger log.Logger | ||
| group string | ||
| } | ||
|
|
||
| func (h goKitHandler) Enabled(_ context.Context, level slog.Level) bool { | ||
| x, ok := h.logger.(leveledLogger) | ||
| if !ok { | ||
| return true | ||
| } | ||
|
|
||
| l := x.level() | ||
| switch level { | ||
| case slog.LevelInfo: | ||
| return l <= infoLevel | ||
| case slog.LevelWarn: | ||
| return l <= warnLevel | ||
| case slog.LevelError: | ||
| return l <= errorLevel | ||
| default: | ||
| return l <= debugLevel | ||
| } | ||
| } | ||
|
|
||
| func (h goKitHandler) Handle(_ context.Context, record slog.Record) error { | ||
| var logger log.Logger | ||
| switch record.Level { | ||
| case slog.LevelInfo: | ||
| logger = level.Info(h.logger) | ||
| case slog.LevelWarn: | ||
| logger = level.Warn(h.logger) | ||
| case slog.LevelError: | ||
| logger = level.Error(h.logger) | ||
| default: | ||
| logger = level.Debug(h.logger) | ||
| } | ||
|
|
||
| if h.group == "" { | ||
| pairs := make([]any, 0, record.NumAttrs()+2) | ||
| pairs = append(pairs, "msg", record.Message) | ||
| record.Attrs(func(attr slog.Attr) bool { | ||
| pairs = append(pairs, attr.Key, attr.Value) | ||
| return true | ||
| }) | ||
| return logger.Log(pairs...) | ||
| } | ||
|
|
||
| pairs := make([]any, 0, record.NumAttrs()) | ||
| record.Attrs(func(attr slog.Attr) bool { | ||
| pairs = append(pairs, attr.Key, attr.Value) | ||
| return true | ||
| }) | ||
| g := slog.Group(h.group, pairs...) | ||
| pairs = []any{"msg", record.Message, g.Key, g.Value} | ||
| return logger.Log(pairs...) | ||
| } | ||
|
|
||
| func (h goKitHandler) WithAttrs(attrs []slog.Attr) slog.Handler { | ||
| pairs := make([]any, 0, len(attrs)) | ||
| for _, attr := range attrs { | ||
| pairs = append(pairs, attr.Key, attr.Value) | ||
| } | ||
|
|
||
| if h.group == "" { | ||
| return goKitHandler{logger: log.With(h.logger, pairs...)} | ||
| } | ||
|
|
||
| g := slog.Group(h.group, pairs...) | ||
| return goKitHandler{logger: log.With(h.logger, g.Key, g.Value)} | ||
| } | ||
|
|
||
| func (h goKitHandler) WithGroup(name string) slog.Handler { | ||
| if name == "" { | ||
| return h | ||
| } | ||
|
|
||
| h.group = name | ||
| return h | ||
| } |
There was a problem hiding this comment.
I'm not sure what kind of tolerances are required for mimir, but I believe the current implementation may be a bit naive. For instance, the WithGroup docs on the slog.Handler state:
// WithGroup returns a new Handler with the given group appended to
// the receiver's existing groups.
It is indeed possible to have more than 1 group; here's output from a contrived example (playground):
time=2009-11-10T23:00:00.000Z level=INFO msg=test g1.g2.hello=world
time=2009-11-10T23:00:00.000Z level=INFO msg=test g1.hello=world g1.hello=world
time=2009-11-10T23:00:00.000Z level=INFO msg=test g1.hello=world g1.g2.hello=world
time=2009-11-10T23:00:00.000Z level=INFO msg=test hello=world hello=world hello=worldHowever, attempting any similar log calls with a logger produced by SlogFromGoKit() panics in the implementation of Handle()
There was a problem hiding this comment.
@tjhop yeah, admittedly I was uncertain about the WithGroup implementation, thanks for producing a failing example. Are you able to identify any problems with the other goKitHandler methods?
There was a problem hiding this comment.
Seems I might've been too hasty in reading the panic message, it actually seems the panic was related to me half-assing the mock logger calls when first playing with this. Better mocking out the same contrived log statements as that playground example, I get this from the handler:
level=info msg=test g2="[hello=world]"
level=info g1="[hello=world]" msg=test hello=world
level=info g1="[hello=world]" msg=test g2="[hello=world]"
level=info hello=world hello=world msg=test hello=world
A few thoughts:
-
slog includes the time by default -- currently the implementation doesn't forward the timestamp from the record along into the kv pairs that get passed to
logger.Log() -
ordering is a bit different, but given the purpose of structured logging, I'd argue it's fine
-
like we already acknowledged, grouping needs some adjustment to properly nest. this patch gets us a bit closer, but still needs further tweaking:
~/go/src/github.com/grafana/mimir (arve/upgrade-mimir-prometheus [ U ]) -> gd pkg/util/log/slogadapter.go diff --git a/pkg/util/log/slogadapter.go b/pkg/util/log/slogadapter.go index 66f084dce3..381a7cdcae 100644 --- a/pkg/util/log/slogadapter.go +++ b/pkg/util/log/slogadapter.go @@ -93,6 +93,11 @@ func (h goKitHandler) WithGroup(name string) slog.Handler { return h } - h.group = name + if h.group == "" { + h.group = name + } else { + h.group = h.group + "." + name + } + return h }level=info msg=test g1.g2="[hello=world]" level=info g1="[hello=world]" msg=test hello=world level=info g1="[hello=world]" msg=test g2="[hello=world]" level=info hello=world hello=world msg=test hello=world
There was a problem hiding this comment.
@aknuds1 I think I have something pretty functional -- mind if I PR against this branch?
There was a problem hiding this comment.
@tjhop feel free :)
aknuds1
force-pushed
the
arve/upgrade-mimir-prometheus
branch
from
69c0bd2
to
a364680
Compare
aknuds1
force-pushed
the
arve/upgrade-mimir-prometheus
branch
3 times, most recently
from
dae507f
to
78797ed
Compare
|
MQE changes to unblock this are in #9948. |
Signed-off-by: Arve Knudsen <[email protected]>
aknuds1
force-pushed
the
arve/upgrade-mimir-prometheus
branch
from
78797ed
to
d920e20
Compare
…w left-open behaviour in Prometheus 3
…left-open interval behaviour
Signed-off-by: Arve Knudsen <[email protected]>
Thanks @charleskorn - included your changes there into this PR. |
What this PR does
Upgrade to latest mimir-prometheus@main, which syncs in Prometheus 3.0 changes.
NB: I add a
go-kit/logtoslogadapter, since Prometheus now uses the latter.Which issue(s) this PR fixes or relates to
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.