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.

Sign up for GitHub

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

Jump to bottom

Monitoring #62

Merged
merged 34 commits into from
Feb 27, 2025
Merged

Monitoring #62

merged 34 commits into from
Feb 27, 2025

Conversation

intarga
Copy link
Member

@intarga intarga commented Feb 5, 2025 •
edited
Loading

TODO list:

This PR includes:

  • Metrics instrumentation of ingestion
  • node_exporter and postgres_exporter deployment
  • Improvements and bugfixes to related parts of our ansible playbooks
  • Tracing (logging) instrumentation of ingestion

Related changes outside this repo (see PRs linked in TODO list):

Things to note:

  • Unfortunately the Grafana instance IT maintains is read-only, meaning we can't easily edit or experiment with our dashboard. Making changes to the dashboard requires:

    • Exporting the dashboard JSON
    • Setting up your own local Grafana
    • Importing the dashboard
    • Making your changes
    • Exporting again
    • Opening a PR on the departments repo replacing the dashboard file with your newly exported JSON
    • Waiting 2 minutes for their yaml lint pipeline to run (which doesn't even check the things that should be checked in the dashboard)
    • Merging the PR
    • Waiting 15-20 minutes for the deployment to go live

    In my opinion this is an unacceptable edit loop for a visualisation dashboard. I'll leave some feedback for the people at IT who maintain it, but if they aren't receptive, we may eventually want to set up our own.

  • I wanted to include metrics/dashboards using pg_stat_statements which would give us more detailed insights into query performance. Unfortunately the stat_statements collector in latest version of postgres_exporter is broken on Postgres 17. There's already a fix for it in their main branch, but as we don't know when the next release will be, I suggest we ignore this for now, and come back to it when the new version is live. The configuration is already done on our end, so it would just require running a playbook to update postgres_exporter, and making visualisations with the new metrics.

  • I was originally intending to turn on ingestion to test and refine the dashboard (Manuel turned it off in relation to his migration work), but now especially as we don't have stat_statements yet, I think this can wait and be bundled into my upcoming benchmarking work, as that will stress the dashboard anyway.

@intarga intarga force-pushed the monitoring branch 4 times, most recently from 3f2df96 to 8c8dce7 Compare February 12, 2025 12:58
@intarga intarga force-pushed the monitoring branch 2 times, most recently from f9cc4ec to a02184b Compare February 18, 2025 16:07
intarga added 24 commits February 19, 2025 16:22
@intarga
set up metrics exporter
d6b3fa1
@intarga
add initial ingestor metrics
2153168
@intarga
add security group for metrics, fix bug with group vars dir name
b0fdf24
@intarga
ansible: fix typos
67479ad
@intarga
ansible: replace keypairs to work around hashing bug
ececf8d
@intarga
register metrics at startup
069f4bf
@intarga
fix rebase dependency breakage
ed755b1
@intarga
ansible: rework security group task to be declarative
4aa1bd8 
The previous version had a footgun that rules removed from the ansible
vars would not be removed by ansible from ostack. This version also
allows our vars to match the structure expected by the ostack collection
@intarga
ansible: add node and postgres metrics exporters
c6dbe86
@intarga
ansible: fix wrong user in postgres_exporter config
7a2b1b3
@intarga
ansible: enable stat_statements collector in postgres_exporter
f49716b
@intarga
ingestion: add histogram metric for request latency
07792b8
@intarga
ingestion: add kafka_failures metric
4e0a559
@intarga
ansible: idempotentize install_postgres.yml
99c56cd
@intarga
ansible: update ostack python deps
1c3282b
@intarga
ansible: add missing region configs to teardown.yml
c04850d
@intarga
ansible: bring back replace based pg conf changes where needed
5f886fc
@intarga
ansible ensure config applied and pg running at end of install_postgr…
65b0207 
…es.yml
@intarga
ansible: disable pg tasks that can't run on replicas
dabb502
@intarga
ansible: improve service handling in pg role
2ca7164
@intarga
ansible: fix wrong boolean operators
5734528
@intarga
ansible: idempotentize the conf file changes by checking the file first
c8cb411
@intarga
ansible: handle replication check failing on first run
6852d08
@intarga
ansible: enable pg_stat_statements
433f492
@intarga
ingestion: set up tracing
f2139aa
@intarga
.github: add ansible lint to ci
373c236
@intarga
.github: merge ci and ansible workflows, fix ansible-lint
90c872f 
These need to both always run because github doesn't allow you to put
conditions on requiring check passes for branch protection
@intarga intarga marked this pull request as ready for review February 20, 2025 13:48
@Lun4m
Copy link
Collaborator

Lun4m commented Feb 21, 2025 •
edited
Loading

  • Waiting 15-20 minutes for the deployment to go live

In my opinion this is an unacceptable edit loop for a visualisation dashboard. I'll leave some feedback for the people at IT who maintain it, but if they aren't receptive, we may eventually want to set up our own.

20 minutes for changing some graphs sounds rough, it's probably faster to do it by hand with the GUI.

I wanted to include metrics/dashboards using pg_stat_statements which would give us more detailed insights into query performance. Unfortunately the stat_statements collector in latest version of postgres_exporter is broken on Postgres 17. There's already a fix for it in their main branch, but as we don't know when the next release will be, I suggest we ignore this for now, and come back to it when the new version is live. The configuration is already done on our end, so it would just require running a playbook to update postgres_exporter, and making visualisations with the new metrics.

It seems like they are merging it soon (today even 🎉 )

I was originally intending to turn on ingestion to test and refine the dashboard (Manuel turned it off in relation to his migration work), but now especially as we don't have stat_statements yet, I think this can wait and be bundled into my upcoming benchmarking work, as that will stress the dashboard anyway.

That makes sense, plus we probably also need to merge the other two open draft PRs before that.

Lun4m
Lun4m requested changes Feb 21, 2025
View reviewed changes
Copy link
Collaborator

@Lun4m Lun4m left a comment

Choose a reason for hiding this comment

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

Nice, it's so satisfying to see that the dashboard is running!

@Lun4m Lun4m linked an issue Feb 21, 2025 that may be closed by this pull request
Monitoring #52
Closed
jo-asplin-met-no
jo-asplin-met-no reviewed Feb 24, 2025
View reviewed changes
@intarga intarga merged commit 2a74653 into trunk Feb 27, 2025
3 checks passed
@intarga intarga deleted the monitoring branch February 27, 2025 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jo-asplin-met-no jo-asplin-met-no jo-asplin-met-no left review comments

@Lun4m Lun4m Lun4m approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Monitoring
3 participants
@intarga @Lun4m @jo-asplin-met-no