Switch to slog #240
Switch to slog #240
Conversation
lucacome
force-pushed
the
slog
branch
3 times, most recently
from
4cc3e71 to
bade6e2
Compare
|
At a minimum, I'm going to cut a new release before merging this. (#246) |
|
@roidelapluie What do you think? Should we just make this a breaking change? Or should we make a new compatible function? |
I think a new compat function would be better. A lot of repos use this. |
|
Aren't breaking changes kind of expected for this repo? I feel like both the version
I also feel like it's not that hard of a change to make for users and it would reduce the number of dependencies. |
Signed-off-by: Luca Comellini <[email protected]>
There was a problem hiding this comment.
I think we should go ahead with this as a breaking change. It's an opinionated library used mostly by us and our exporters.
If we get a lot of complaints, we can make a decision to revert.
We currently have the people and the momentum to migrate stuff. I'm planning to publish a helper script for converting go-kit/log calls to slog.
|
@roidelapluie what do you think? |
|
OK! |
* [CHANGE] Switch logging library to slog #240 Signed-off-by: SuperQ <[email protected]>
* [CHANGE] Switch logging library to slog #240 Signed-off-by: SuperQ <[email protected]>
|
Despite this package being pre-1.0 and thus entitled to make breaking changes, this will nevertheless be quite inconvenient for packaging in Debian & derivatives, due to their lack of support for multiple concurrent versions of libraries. Packaging exporter-toolkit v0.13.0 will break approximately 25 exporters in Debian which currently depend on it. Patching all 25 exporters will be considerable work, not to mention the turnaround time of getting all those patches pushed to their respective upstream projects. I have put some thought into how exporter-toolkit could have made this change non-breaking, and came up with this: func ListenAndServe(server *http.Server, flags *FlagConfig, origLogger any) error {
var logger *slog.Logger
switch origLogger := origLogger.(type) {
case log.Logger:
// Re-parsing the command line is the only way to determine the go-kit log config, since
// nothing useful is exported by go-kit/log.Logger.
// Since promslog uses exactly the same flag names and options as promlog, we can take a
// shortcut and parse the args directly into a promslog.Config.
app := kingpin.New("", "")
config := &promslog.Config{Style: promslog.GoKitStyle}
flag.AddFlags(app, config)
kingpin.MustParse(app.Parse(os.Args[1:]))
logger = promslog.New(config)
case *slog.Logger:
logger = origLogger
default:
panic("unsupported logger type")
}
...As a maintainer of the golang-github-prometheus-exporter-toolkit package in Debian, I will implement this as a patch to ease the transition for all the "legacy" exporters which still use go-kit/log. However, if there is interest here, I can submit a PR to have this workaround implemented here for the benefit of all. |
|
We're planning to be rolling out the exporter-toolkit logging change reasonably quickly and cutting new exporter releases. If we did this over the next ~month, would that be reasonable for updating Debian? For example, I've already done this in the node_exporter (prometheus/node_exporter#3097). Otherwise, we could implement the |
|
Debian packages a lot of third-party exporters that are not under the maintenance umbrella of the Prometheus community. Some of these exporters rarely get updated by their developers. I have personally opened quite a few PRs against the upstream projects, nudging them to modernize, so that Debian does not have to maintain such a large patch set. One such example was the switch from logrus to go-kit logging just a few years ago. Whilst I have faith that you can update many of the core Prometheus exporters in a reasonable timeframe, my concern is mainly the multitude of third-party exporters, some of which are even borderline unmaintained. |
|
@dswarbrick do you have a list of those projects? I wouldn’t mind helping out. |
|
@lucacome Not specifically, but https://packages.debian.org/search?suite=sid&keywords=prometheus is a good starting point. Note that not all of the exporters are written in Go, but I reckon probably about two thirds of them are, and of those, the vast majority use exporter-toolkit. |
|
A couple of other things that have since caught my eye about this change, which may cause minor issues with users who have configured log parsers (e.g. Loki) to read exporter logs:
|
|
@dswarbrick Yes, those changes are intentional. We want to switch to slog. So switching to slog's standard formatting is the default. The upstream |
|
@lucacome I just built a local golang-github-prometheus-exporter-toolkit 0.13.0 package for Debian, and used ratt to identify reverse dependencies and check their builds. Of the 23 packages in sid which currently depend on exporter-toolkit, 22 of them failed to build (unsurprisingly). I will go ahead with my plan to patch exporter-toolkit in Debian so that it will accept a legacy go-kit logger as an interim workaround, as I don't currently have the time to migrate 22 exporters to log/slog. |
The Legacy promlog package will be removed in the next prometheus/common release. procfs and client_golang will also be updated transitively. prometheus/common#677 prometheus/exporter-toolkit#240
Switch to log/slog to align with prometheus/common#677