prometheus / exporter-toolkit

Utility package to build exporters
Apache License 2.0
265 stars 81 forks source link

Switch to slog #240

Closed lucacome closed 1 month ago

lucacome commented 2 months ago

Switch to log/slog to align with https://github.com/prometheus/common/pull/677

SuperQ commented 2 months ago

At a minimum, I'm going to cut a new release before merging this. (https://github.com/prometheus/exporter-toolkit/pull/246)

SuperQ commented 2 months ago

@roidelapluie What do you think? Should we just make this a breaking change? Or should we make a new compatible function?

roidelapluie commented 2 months ago

@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.

lucacome commented 2 months ago

Aren't breaking changes kind of expected for this repo?

I feel like both the version 0.x.x and the note that is still in the README signal that

This repository is currently WIP and experimental.

I also feel like it's not that hard of a change to make for users and it would reduce the number of dependencies. But I'm happy to make changes to use a compact function, if you think that's better.

SuperQ commented 1 month ago

@roidelapluie what do you think?

roidelapluie commented 1 month ago

OK!

dswarbrick commented 1 month ago

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.

SuperQ commented 1 month ago

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 (https://github.com/prometheus/node_exporter/pull/3097).

Otherwise, we could implement the any handler here.

dswarbrick commented 1 month ago

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.

lucacome commented 1 month ago

@dswarbrick do you have a list of those projects? I wouldn’t mind helping out.

dswarbrick commented 1 month ago

@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.

dswarbrick commented 1 month ago

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:

SuperQ commented 1 month ago

@dswarbrick Yes, those changes are intentional. We want to switch to slog. So switching to slog's standard formatting is the default. The upstream promslog allows for a backwards compatible format, but we decided not to use it here, as this is an opinionated library.

dswarbrick commented 1 month ago

@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).

2024/09/19 17:27:42 Build results:
2024/09/19 17:27:42 PASSED: alertmanager-irc-relay
2024/09/19 17:27:42 FAILED: prometheus-pgbackrest-exporter (see buildlogs/prometheus-pgbackrest-exporter_0.18.0+ds1-1)
2024/09/19 17:27:42 FAILED: prometheus-mqtt-exporter (see buildlogs/prometheus-mqtt-exporter_0.1.7-1)
2024/09/19 17:27:42 FAILED: prometheus-mysqld-exporter (see buildlogs/prometheus-mysqld-exporter_0.15.1-1)
2024/09/19 17:27:42 FAILED: prometheus-alertmanager (see buildlogs/prometheus-alertmanager_0.27.0+ds-2)
2024/09/19 17:27:42 FAILED: prometheus-blackbox-exporter (see buildlogs/prometheus-blackbox-exporter_0.25.0-1)
2024/09/19 17:27:42 FAILED: prometheus-ipmi-exporter (see buildlogs/prometheus-ipmi-exporter_1.8.0-1)
2024/09/19 17:27:42 FAILED: prometheus (see buildlogs/prometheus_2.45.6+ds-1)
2024/09/19 17:27:42 FAILED: prometheus-hacluster-exporter (see buildlogs/prometheus-hacluster-exporter_1.3.3-1)
2024/09/19 17:27:42 FAILED: prometheus-elasticsearch-exporter (see buildlogs/prometheus-elasticsearch-exporter_1.7.0-2)
2024/09/19 17:27:42 FAILED: prometheus-postgres-exporter (see buildlogs/prometheus-postgres-exporter_0.15.0-3)
2024/09/19 17:27:42 FAILED: prometheus-pushgateway (see buildlogs/prometheus-pushgateway_1.7.0-2)
2024/09/19 17:27:42 FAILED: prometheus-smokeping-prober (see buildlogs/prometheus-smokeping-prober_0.8.1-3)
2024/09/19 17:27:42 FAILED: prometheus-haproxy-exporter (see buildlogs/prometheus-haproxy-exporter_0.15.0-2)
2024/09/19 17:27:42 FAILED: prometheus-snmp-exporter (see buildlogs/prometheus-snmp-exporter_0.26.0-1)
2024/09/19 17:27:42 FAILED: prometheus-apache-exporter (see buildlogs/prometheus-apache-exporter_1.0.7-1)
2024/09/19 17:27:42 FAILED: prometheus-bind-exporter (see buildlogs/prometheus-bind-exporter_0.7.0-3)
2024/09/19 17:27:42 FAILED: prometheus-squid-exporter (see buildlogs/prometheus-squid-exporter_1.12.0-1)
2024/09/19 17:27:42 FAILED: prometheus-process-exporter (see buildlogs/prometheus-process-exporter_0.8.3-1)
2024/09/19 17:27:42 FAILED: prometheus-node-exporter (see buildlogs/prometheus-node-exporter_1.8.2-1)
2024/09/19 17:27:42 FAILED: golang-github-prometheus-community-pgbouncer-exporter (see buildlogs/golang-github-prometheus-community-pgbouncer-exporter_0.9.0-1)
2024/09/19 17:27:42 FAILED: prometheus-nginx-exporter (see buildlogs/prometheus-nginx-exporter_1.3.0-1)
2024/09/19 17:27:42 FAILED: prometheus-frr-exporter (see buildlogs/prometheus-frr-exporter_1.3.1-1)

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.