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

Updated ScyllaDB Prometheus dashboard and documentation #588

Conversation

algchoo
Copy link
Contributor

@algchoo algchoo commented Jul 14, 2023

Changes

Details
In version 5.2 of ScyllaDB, the following metrics are no longer present

  • scylla_storage_proxy_coordinator_writes_coordinator_outside_replica_set (Write Requests)
  • scylla_storage_proxy_coordinator_write_timeouts (Write Timeouts)
  • scylla_storage_proxy_coordinator_read_timeouts (Read Timeouts)

The Read Performance and Write Performance both have the timeout metrics removed, while an equivalent has replaced the scylla_storage_proxy_coordinator_writes_coordinator_outside_replica_set (Write Requests) metric:

Description: total number of write requests on a local Node
Metric name: scylla_storage_proxy_coordinator_total_write_attempts_local_node

The minimum version has been updated to 5.2 in the documentation and the metadata.

@github-actions github-actions bot requested a review from varun-c July 14, 2023 19:21
Copy link
Collaborator

@yqlu yqlu left a comment

Choose a reason for hiding this comment

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

Flagging to @EvanSimpson as a second breaking change case, even though the first such case (#581 for postgres) may be a false alarm

@@ -4,7 +4,7 @@ platforms:
exporter_metadata:
name: ScyllaDB Prometheus Exporter
doc_url: https://monitoring.docs.scylladb.com/stable/reference/monitoring_apis.html
minimum_supported_version: "5.0"
minimum_supported_version: "5.2"
detections:
- characteristic_metric:
Copy link
Collaborator

Choose a reason for hiding this comment

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

We'll need to update the list of default metrics below too

"scale": "LINEAR"
}
}
},
"width": 12,
"xPos": 0,
"yPos": 28
"yPos": 7
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this mean we have to retake the screenshot preview of the dashboard in the same folder?

@EvanSimpson
Copy link
Collaborator

Flagging to @EvanSimpson as a second breaking change case, even though the first such case (#581 for postgres) may be a false alarm

Does the dashboard currently query the metrics which are going away? If the dashboard is fully backwards-compatible with both 5.0 and 5.2 metrics, I think we are good to bump the version and update the list of default metrics to match what 5.2 collects.

If not, we should consider a new integration version with a forked dashboard.

@algchoo
Copy link
Contributor Author

algchoo commented Aug 2, 2023

@EvanSimpson Apologies for not answering sooner, the dashboard does currently query those metrics that are going away. I believe that means it won't be backwards-compatible with both versions. I think what you've suggested will be the best path forward.

google-nalin and others added 22 commits August 14, 2023 15:23
* updated min exporter version to be the latest version of Velero supported by the GCP plugin for Velero

* the plugin support matrix doesn't match what is deployed, changed the minimum supported version

* updated the minimum exporter version to be 1.7.1
* removed args for the exporter due to them all being default values, updated min version

* updated the args to include web.listen-address which replaced telemetry.address

* updated launch_stage to GA for apache http metadata
* removed deprecated metric and updated detection metric

* removed deprecated metric from chart, updated screenshot
* Updated haproxy config, port name, and image version. Updated minimum version to match haproxy version

* Changed haproxy exporter type to be 'included' and updated the documentation to fit that template

* added back deployment yaml, named port, and metric verification instructions

* added class to links, added new lines

* small wording changes
)

Specifically, acknowledge that Airflow itself doesn't expose metrics, but Airflow's Helm chart automatically includes a StatsD deployment that does expose metrics out of the box. This matches the Airflow metrics docs and the Airflow Helm chart docs.
* updated the minimum exporter version, example configuration, and additional install info

* updated the minimum supported version in metadata

* update config example to pass a config file as an argument

* reverted min versions, added volume to the config example that mounts the mysql exporter config

* added + symbols for configmap in example yaml

* removed a few dashes, updated the volume section, added mysql image to container section
@algchoo
Copy link
Contributor Author

algchoo commented Aug 15, 2023

@EvanSimpson I've pushed updates to version the dashboards, please let me know if there are any changes necessary. Hope you're having a great day!

@EvanSimpson
Copy link
Collaborator

@algchoo thanks! It looks like there are a lot of unrelated changes in this PR, does this branch need to be rebased?

@robinreneemac727

This comment was marked as spam.

@algchoo
Copy link
Contributor Author

algchoo commented Aug 24, 2023

@algchoo thanks! It looks like there are a lot of unrelated changes in this PR, does this branch need to be rebased?

I've gone ahead and rebased after making sure our fork was up to date, it should be good to go now. Hope you're having a good day!

@algchoo
Copy link
Contributor Author

algchoo commented Aug 24, 2023

@EvanSimpson I still see a handful of changes that weren't part of what I was doing here, maybe I rebased incorrectly? On the plus side, there are no longer conflicts.

@EvanSimpson EvanSimpson changed the base branch from master to airflow-revision August 24, 2023 18:43
@EvanSimpson EvanSimpson changed the base branch from airflow-revision to master August 24, 2023 18:43
@EvanSimpson
Copy link
Collaborator

@EvanSimpson I still see a handful of changes that weren't part of what I was doing here, maybe I rebased incorrectly? On the plus side, there are no longer conflicts.

I tried the quick fix described here to change the target to something else and then back but it doesn't seem to have worked https://stackoverflow.com/questions/16306012/github-pull-request-showing-commits-that-are-already-in-target-branch

Can you try one of the options outlined in this answer? https://stackoverflow.com/a/45286781

@algchoo
Copy link
Contributor Author

algchoo commented Aug 24, 2023

@EvanSimpson I've opened a new PR to see if that helps solve this problem, here's a link and I'll close this PR.

@algchoo algchoo closed this Aug 24, 2023
@robinreneemac727
Copy link

robinreneemac727 commented Sep 15, 2023 via email

@robinreneemac727
Copy link

robinreneemac727 commented Sep 15, 2023 via email

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.

10 participants