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

Send Prometheus metrics to Grafana Cloud #2994

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

Conversation

ndr-ds
Copy link
Contributor

@ndr-ds ndr-ds commented Dec 2, 2024

Motivation

We're migrating our internal validators to send data to Grafana Cloud.

Proposal

Starting with the local setup. This was useful to test the required changes that we'll likely need for the actual internal validator setup. We probably won't want to send metrics to Grafana Cloud from local runs all the time (specially for cost reasons), so this is disabled by default.

Grafana Cloud credentials are stored in a secret within GCP's Secret Manager. Since Grafana Cloud will be for our internal validators, then I figured storing the secret on GCP made sense.

Test Plan

Ran locally with linera net up --kubernetes, and saw the metrics in our Grafana Cloud instance:

Screenshot 2024-12-02 at 11.37.04.png

Release Plan

If we want to start sending metrics to Grafana Cloud from our devnet/testnet, then:

  • These changes should be backported to the latest devnet branch
  • These changes should be backported to the latest testnet branch, then

@ndr-ds ndr-ds requested review from christos-h, jvff, ma2bd and Twey December 2, 2024 14:48
@ndr-ds ndr-ds force-pushed the 12-02-send_prometheus_metrics_to_grafana_cloud branch from 00f28a0 to 4aa0e5d Compare December 2, 2024 17:52
@ndr-ds ndr-ds force-pushed the 11-28-avoid_build_on_linera_net_up_--kubernetes branch from a872d17 to 37758e3 Compare December 2, 2024 21:11
@ndr-ds ndr-ds force-pushed the 12-02-send_prometheus_metrics_to_grafana_cloud branch from 4aa0e5d to e11c5b7 Compare December 2, 2024 21:11
Copy link
Contributor

@christos-h christos-h 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 the cleanup.

I'm a bit confused on two things:

  1. Do we not still want the internal Grafana which is validator specific? In which case why is the dashboard disabled? If you don't want the dashboard why not remove Grafana entirely?
  2. The reference to the GCP secret should not be hard-coded in linera-protocol but instead be parameterised and injected by the caller in linera-infra (probably lineractl).

Copy link
Contributor Author

ndr-ds commented Dec 3, 2024

  1. The way I did it, either you do Grafana Cloud (writeToGrafanaCloud set to true), or you do the same existing setup we have right now. With Grafana Cloud, we don't need the Grafana pod, or the dashboards or any of that. So the idea is that for internal validators we run it with writeToGrafanaCloud set to true, and for external validators we run it with writeToGrafanaCloud set to false.
  2. Ok, sure, I can do that. I thought it was fine since you need to be authenticated with gcloud for the secret to be successfully fetched 🤔

@ndr-ds ndr-ds force-pushed the 11-28-avoid_build_on_linera_net_up_--kubernetes branch from 37758e3 to 1b117c3 Compare December 3, 2024 21:33
@ndr-ds ndr-ds force-pushed the 12-02-send_prometheus_metrics_to_grafana_cloud branch from e11c5b7 to 188dc33 Compare December 3, 2024 21:34
@ndr-ds ndr-ds force-pushed the 11-28-avoid_build_on_linera_net_up_--kubernetes branch 2 times, most recently from 6266ccd to 3650595 Compare December 4, 2024 14:13
@ndr-ds ndr-ds force-pushed the 12-02-send_prometheus_metrics_to_grafana_cloud branch from 188dc33 to 17b1c8c Compare December 4, 2024 14:13
@ndr-ds ndr-ds changed the base branch from 11-28-avoid_build_on_linera_net_up_--kubernetes to graphite-base/2994 December 4, 2024 14:16
@ndr-ds ndr-ds force-pushed the 12-02-send_prometheus_metrics_to_grafana_cloud branch from 17b1c8c to ec0f9af Compare December 4, 2024 14:16
@ndr-ds ndr-ds changed the base branch from graphite-base/2994 to main December 4, 2024 14:17
@ndr-ds ndr-ds force-pushed the 12-02-send_prometheus_metrics_to_grafana_cloud branch from ec0f9af to e9fee28 Compare December 4, 2024 14:17
@ndr-ds ndr-ds force-pushed the 12-02-send_prometheus_metrics_to_grafana_cloud branch from e9fee28 to cc8f74c Compare January 8, 2025 20:55
@ndr-ds ndr-ds requested a review from christos-h January 8, 2025 20:57
@ndr-ds ndr-ds force-pushed the 12-02-send_prometheus_metrics_to_grafana_cloud branch from cc8f74c to f682033 Compare January 10, 2025 14:50
Copy link
Contributor

@christos-h christos-h left a comment

Choose a reason for hiding this comment

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

Left some comments. Also the Kubernetes workflow is not passing. Although this may be unrelated to these changes.

name: grafana-cloud-auth-secret
type: kubernetes.io/basic-auth
stringData:
username: {{ .Values.grafanaCloudUsername | quote }}
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if this isn't specified (for example external validators which use Kuberentes?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm currently changing this to only be created if writeToGrafanaCloud is true, which will only happen for internal validators

@@ -1,12 +1,12 @@
# Values for charts linera-validator for local validators.

# Linera
lineraImage: "" # Is set by helmfile.
lineraImage: {{ env "LINERA_HELMFILE_LINERA_IMAGE" | default "linera:latest" }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for this. I'm not entirely sure if this is better or worse to be honest. On the one hand there is less indirection on the other hand configurable variables are now in more places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it definitely looks better/less confusing than just an empty string, tbh 🤔 at least now you know what the default for this is supposed to be

Copy link
Contributor Author

ndr-ds commented Jan 10, 2025

It's related, actually. Working on a fix right now!

@ndr-ds ndr-ds force-pushed the 12-02-send_prometheus_metrics_to_grafana_cloud branch from b8d7e85 to 2001d5c Compare January 10, 2025 19:20
@ndr-ds ndr-ds force-pushed the 12-02-send_prometheus_metrics_to_grafana_cloud branch from 2001d5c to d158401 Compare January 10, 2025 19:27
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