Skip to content
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

chore(deps): Update the prometheus dependency of Loki and Promtail #12245

Merged
merged 4 commits into from
Mar 21, 2024

Conversation

ptodev
Copy link
Contributor

@ptodev ptodev commented Mar 18, 2024

What this PR does / why we need it:

This PR updates the Prometheus dependency of Loki and Promtail. The PR is required so that we can also update Grafana Agent to the latest Prometheus. Unfortunately, Promtail and Loki share the same go.mod file. I am not able to update only Promtail.

Special notes for your reviewer:

  • Is there a need to do any local testing? Or to add unit tests?
  • Should I update any other go.mod files?
  • The only user facing changes of this PR are that some metrics might change. Is this a problem?
    • Are there any dashboards and alerts which need to be updated?
    • Do I need to add a changelog entry?
    • Any updates to documentation? As far as I can tell, those metrics are not documented anywhere.
    • The metrics in pkg/ruler use a registry which is wrapped by labels such as user ID. I hope the cardinality of SD metrics won't increase too much now that the SD metrics will be using this wrapped registry too.

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • CHANGELOG.md updated
    • If the change is worth mentioning in the release notes, add add-to-release-notes label
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
  • For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR
  • If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

@ptodev ptodev requested a review from a team as a code owner March 18, 2024 14:28
@ptodev ptodev force-pushed the ptodev/update-promtail-prometheus branch from 44839b2 to 5fed91e Compare March 20, 2024 12:30
This commit also does a "go mod tidy" and a "go mod vendor".
@ptodev ptodev force-pushed the ptodev/update-promtail-prometheus branch from 5fed91e to fcc8daa Compare March 20, 2024 17:26
@ptodev ptodev changed the title Update the prometheus dependency of Loki and Promtail chore(deps): Update the prometheus dependency of Loki and Promtail Mar 20, 2024
@ptodev
Copy link
Contributor Author

ptodev commented Mar 20, 2024

@chaudum I tested the Promtail code by running a local Promtail with this config:

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: ""
    basic_auth:
      username: ""
      password: ""
scrape_configs:
- job_name: log files
  consulagent_sd_configs:
  docker_sd_configs:
  file_sd_configs:
  static_configs:
  - labels:
      __path__: /Users/paulintodev/Desktop/log_metrics_bug/*.log

I also ran a Promtail using the code form the main, and compared the metrics which they expose on their http://localhost:9080/metrics endpoints. I found that all prometheus_sd_ and prometheus_target_ metrics are missing:

This is a correct side-effect of the latest Prometheus code. Promtail doesn't even use the Prometheus components which output those metrics, so I think it's safe to remove them.

I will need you to confirm a few things please:

  • Given that the metrics we are removing are not used, I believe there is no need to update the docs?
  • I am not sure if the changelog entry for this change will be descriptive enough. I guess I should mention in my commits that metrics which Promtail exposed are being removed?
  • It would be nice if we could run a ruler, just to confirm that it runs ok and that its metrics haven't changed. Would this be very difficult?

@ptodev ptodev force-pushed the ptodev/update-promtail-prometheus branch 3 times, most recently from 344aa54 to 65e8b33 Compare March 20, 2024 18:40
This is no longer needed, because the code was copied to Prometheus:
"github.com/prometheus/prometheus/storage/remote/otlptranslator/prometheus"
@ptodev ptodev force-pushed the ptodev/update-promtail-prometheus branch from 65e8b33 to 5c501ad Compare March 20, 2024 19:15
@ptodev ptodev force-pushed the ptodev/update-promtail-prometheus branch from 5c501ad to 0c2a2c7 Compare March 20, 2024 19:41
Copy link
Contributor

@chaudum chaudum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tackling the Prometheus dependency upgrade!

@chaudum chaudum merged commit d54e087 into main Mar 21, 2024
10 checks passed
@chaudum chaudum deleted the ptodev/update-promtail-prometheus branch March 21, 2024 10:47
edsoncelio pushed a commit to edsoncelio/loki that referenced this pull request Mar 22, 2024
This PR updates the Prometheus dependency in Loki and Promtail. The PR is required so that we can also update Grafana Agent to the latest Prometheus. Unfortunately, Promtail and Loki share the same go.mod file.
rhnasc pushed a commit to inloco/loki that referenced this pull request Apr 12, 2024
This PR updates the Prometheus dependency in Loki and Promtail. The PR is required so that we can also update Grafana Agent to the latest Prometheus. Unfortunately, Promtail and Loki share the same go.mod file.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants