fix: promtail; clean up metrics generated from logs after a config reload.

[ptodev]

It is possible to generate metrics from log lines in Promtail. If the config file is reloaded, those metrics are currently not cleaned up. This causes a few issues:

• If a metric is not necessary anymore, it is still visible on the /metrics endpoint.
• A metric with the same name might have a different meaning after the config file reload, so it'd make sense to clean these up.

This will help us fix a bug reported in the Grafana Agent.

I suppose no changelog entry is required because PromQL is already designed to handle counter resets? But if you think it's appropriate, I could add a changelog entry?

I tested this change locally using a config file like this:

Promtail config

server:
  http_listen_port: 9080
  grpc_listen_port: 0
  enable_runtime_reload: true
positions:
  filename: /Users/paulintodev/Desktop/log_metrics_bug/tmp_promtail/positions
clients:
  - url: "https://logs-prod-008.grafana.net/loki/api/v1/push"
    basic_auth:
      username: ""
      password: ""
scrape_configs:
- job_name: log files
  static_configs:
  - labels:
      __path__: /Users/paulintodev/Desktop/log_metrics_bug/*.log
  pipeline_stages:
    - metrics:
        log_lines_total:
          type: Counter
          description: "total number of log lines"
          prefix: my_promtail_custom_
          max_idle_duration: 24h
          config:
            match_all: true
            action: inc
        log_lines_total2:
          type: Counter
          description: "total number of log lines"
          prefix: my_promtail_custom2_
          max_idle_duration: 24h
          config:
            match_all: true
            action: inc

Then I'd do a curl localhost:9080/reload and check http://localhost:9080/metrics.

I didn't add a whole lot of unit tests because TBH we should probably focus on adding tests in the Agent, given that we're sunsetting Promtail over time.

[CLAassistant]

All committers have signed the CLA.

[cstyan]

@ptodev need to rebase/merge in master to get some CI updates

[cstyan]

shouldn't we specifically check for the absence of the metric?

[ptodev]

After pl.Cleanup(), no metrics should be visible. I don't see an advantage in checking individual metrics when there shouldn't be any of them.

[cstyan]

what if there's originally two metrics stages, and after we reload there should be only one? we should still check for it's existence and correct value, I think right now we're deleting metrics for stages that will still exist after the reload which would not be correct

[ptodev]

I'd argue that deleting all the metrics is the right thing to do. The fact that a metric exists doesn't mean that it's used in the same way in the new config. It's possible that the user changed the meaning of each metric, but retained the metric names. In that case it would make sense to start with fresh metrics.

In the future we could make the code smart and reload only the stages which changed, but I think that's a separate issue. To do that, we could avoid calling the Cleanup method for stages which are completely unchanged as a whole.

[cstyan]

we should document this clearly in the metrics config section

loki/docs/sources/send-data/promtail/configuration.md

Line 678 in a00f1f1

#### metrics

and maybe as a note here as well

[ptodev]

I added this line to both doc pages:

If Promtail's configuration is reloaded, all metrics will be reset.

[cstyan]

@ptodev sorry for the delay, I think we just need some docs, also have you guys been using this code downstream in Agent/Alloy already or would merging this be the first usage of this code?

[cstyan]

we should document this clearly in the metrics config section

loki/docs/sources/send-data/promtail/configuration.md

Line 678 in a00f1f1

#### metrics

and maybe as a note here as well

[ptodev]

have you guys been using this code downstream in Agent/Alloy already or would merging this be the first usage of this code?

Thank you for the review! This is the first usage. Alloy and Agent import the Promtail code, so in order to update them I will need to merge this PR first.

[cstyan]

I'm going to approve/merge this with a note that if we get reports of issues I'll revert this change. Hopefully we here from the Alloy team after a short period of time whether they have confirmed usages that are working well or not.