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

Export Traefik metrics to Prometheus and add Grafana dashboard for them #3688

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ginta1337
Copy link
Contributor

Introduce matrix_prometheus_traefik_exporter_enabled variable. Setting it to true enables export Traefik metrics to Prometheus and adds Grafana dashboard for them.

@spantaleev
Copy link
Owner

I'm not sure what you're doing here and why.

You seem to be replacing matrix_prometheus_nginxlog_exporter_enabled which toggles the matrix-prometheus-nginxlog-exporter Ansible role (which installs the martin-helmich/prometheus-nginxlog-exporter component) with some new other variable (matrix_prometheus_traefik_exporter_enabled) which does other things.

While Traefik is the reverse-proxy that the playbook uses nowadays, nginx is still used in one or more services (like matrix-synapse-reverse-proxy-companion - auto-enabled if Synapse workers are enabled). Using prometheus-nginxlog-exporter provides a way to observe this service.

This PR seems to be adding something additional (the ability to observe Traefik metrics via Prometheus), but:

  • it breaks prometheus-nginxlog-exporter installation
  • it introduces a matrix_prometheus_traefik_exporter_enabled variable which I'm not sure is necessary

I suppose there's no need for matrix_prometheus_traefik_exporter_enabled. We can tell people to flip traefik_config_metrics_prometheus_enabled instead and to have that automatically cascade into modifying matrix_prometheus_services_connect_scraper_traefik_enabled, etc.

For other components, grafana_dashboard_download_urls is redefined in group_vars/matrix_servers and various dashboards are auto-enabled based on "metrics enabled" variables in the various Ansible roles:

grafana_dashboard_download_urls: |
{{
(matrix_synapse_grafana_dashboard_urls if matrix_homeserver_implementation == 'synapse' and matrix_synapse_metrics_enabled else [])
+
(prometheus_node_exporter_dashboard_urls if prometheus_node_exporter_enabled else [])
+
(prometheus_postgres_exporter_dashboard_urls if prometheus_postgres_exporter_enabled else [])
+
(matrix_prometheus_nginxlog_exporter_dashboard_urls if matrix_prometheus_nginxlog_exporter_enabled else [])
+
(matrix_media_repo_dashboard_urls if matrix_media_repo_metrics_enabled else [])
+
(matrix_synapse_usage_exporter_dashboard_urls if matrix_synapse_usage_exporter_enabled else [])
}}

The same can be done for Traefik. If traefik_config_metrics_prometheus_enabled, then some _dashboard_urls variable (containing Traefik-related dashboards) can be injected into grafana_dashboard_download_urls. You have currently defined these dashboard URLs in matrix_traefik_dashboard_download_* variables. While it's nice that this allows extra configurability as to whether dashboard download should happen or not (which is not the case for the other components), it would probably be nice if the Traefik dashboard URL lived (was exported by) the Traefik role itself. This also makes it easier for people using these roles independently (or in the context of mash-playbook) to easily add dashboards to their Grafana instance.

mash-playbook does not automatically populate grafana_dashboard_download_urls like we do for matrix-docker-ansible-deploy. For mash-playbook, we've explicitly decided to avoid such wiring magic. The only wiring magic there is the auto-configuration of Postgres/MariaDB. It's up to the user to wire other services together as necessary. Still, it 's helpful if roles expose _dashboard_urls* variables that can be used for people wishing to do this wiring.

@ginta1337
Copy link
Contributor Author

The purpose of this pull request is to streamline the configuration for users who may not be highly familiar with Ansible. By introducing matrix_prometheus_traefik_exporter_enabled, we provide a simple toggle to enable Traefik metrics export to Prometheus and to automatically add an associated Grafana dashboard.

My point are:

  • Simplifying Configuration for Less Experienced Users: Introducing matrix_prometheus_traefik_exporter_enabled makes it easier for users to configure Prometheus and Grafana integrations for Traefik, without needing to adjust multiple role variables like: traefik_config_metrics_prometheus_enabled, traefik_container_metrics_host_bind_port , traefik_container_additional_networks_custom, prometheus_config_scrape_configs_additional and redefining grafana_dashboard_download_urls in host_vars. This approach aligns with the intent behind matrix_prometheus_nginxlog_exporter_enabled - to encapsulate the setup within a single variable toggle.

  • Functional Similarity to matrix_prometheus_nginxlog_exporter_enabled: The functionality enabled by matrix_prometheus_traefik_exporter_enabled is analogous to matrix_prometheus_nginxlog_exporter_enabled, serving to simplify Prometheus and Grafana configurations related to respective services. Using a similar variable structure maintains consistency within the playbook and allows users to manage configurations more intuitively.

  • Documentation Changes Do Not Affect Nginx Exporter: The changes to the documentation clarify the shift towards using Traefik as the primary reverse proxy while retaining the ability to use the nginxlog exporter. There’s no functional breakage in the nginx exporter setup, as the matrix_prometheus_nginxlog_exporter_enabled toggle remains unchanged.

  • Consistency in grafana_dashboard_download_urls: The additions to grafana_dashboard_download_urls follow the same rules as other items, ensuring that the Traefik dashboards integrate is consistent with the pattern used for other components and reduces the chance of misconfigurations for users.

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