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

config: Add Prometheus config #191

Merged
merged 1 commit into from
Oct 5, 2023
Merged

config: Add Prometheus config #191

merged 1 commit into from
Oct 5, 2023

Conversation

ZJvandeWeg
Copy link
Member

The application now features a /metrics endpoint for Prometheus to scrape. Since merging, the configuration file structure has been updated since through another PR.

This change requires both changes to be in the application and afterwards allows enabling the endpoint and an engineer to configure scraping of the data.

@ZJvandeWeg ZJvandeWeg temporarily deployed to stage October 5, 2023 09:30 — with GitHub Actions Inactive
@ZJvandeWeg ZJvandeWeg temporarily deployed to stage October 5, 2023 09:30 — with GitHub Actions Inactive
@ZJvandeWeg ZJvandeWeg temporarily deployed to stage October 5, 2023 09:30 — with GitHub Actions Inactive
@ZJvandeWeg ZJvandeWeg temporarily deployed to stage October 5, 2023 09:33 — with GitHub Actions Inactive
@ZJvandeWeg ZJvandeWeg temporarily deployed to stage October 5, 2023 09:34 — with GitHub Actions Inactive
Copy link
Contributor

@hardillb hardillb left a comment

Choose a reason for hiding this comment

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

Need to check the conditional inclusion in the configmap

Comment on lines 164 to 167
prometheus:
enabled: {{ .Values.forge.telemetry.prometheus.enabled }}
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't get added unless forge.telemtry.sentry is also set (controlled by the if block above and the end under).

Also it looks like the promethus tag should be one level deeper (to match the depth of sentry under backend

I can't suggest a change as it would need to include changes to lines not added by this PR

Something like this should work:

      backend:
        {{ if and (hasKey .Values.forge.telemetry "sentry") (hasKey .Values.forge.telemetry.sentry "backend_dsn") -}}
        sentry:
          dsn: {{ .Values.forge.telemetry.sentry.backend_dsn}}
        {{- end }}
        prometheus:
          enabled: {{ .Values.forge.telemetry.prometheus.enabled }}

Copy link
Member Author

Choose a reason for hiding this comment

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

I also think the denting of prometheus is one level too deep, it's not a subkey of backend but top level. Thanks for catching this. Pushed a change, let me know if this is correct @hardillb

Copy link
Contributor

Choose a reason for hiding this comment

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

@ZJvandeWeg ZJvandeWeg temporarily deployed to stage October 5, 2023 09:46 — with GitHub Actions Inactive
@ZJvandeWeg ZJvandeWeg temporarily deployed to stage October 5, 2023 09:46 — with GitHub Actions Inactive
@ZJvandeWeg ZJvandeWeg temporarily deployed to stage October 5, 2023 09:46 — with GitHub Actions Inactive
@ZJvandeWeg ZJvandeWeg temporarily deployed to stage October 5, 2023 09:49 — with GitHub Actions Inactive
@ZJvandeWeg ZJvandeWeg temporarily deployed to stage October 5, 2023 09:50 — with GitHub Actions Inactive
The application now features a [/metrics endpoint][1] for Prometheus to
scrape. Since merging, the configuration file structure has been updated
since through [another PR][2].

This change requires both changes to be in the application and
afterwards allows enabling the endpoint and an engineer to configure
scraping of the data.

[1]: FlowFuse/flowfuse#2889
[2]: FlowFuse/flowfuse#2893
@ZJvandeWeg ZJvandeWeg temporarily deployed to stage October 5, 2023 10:14 — with GitHub Actions Inactive
@ZJvandeWeg ZJvandeWeg temporarily deployed to stage October 5, 2023 10:14 — with GitHub Actions Inactive
@ZJvandeWeg ZJvandeWeg temporarily deployed to stage October 5, 2023 10:14 — with GitHub Actions Inactive
@ZJvandeWeg ZJvandeWeg temporarily deployed to stage October 5, 2023 10:16 — with GitHub Actions Inactive
@ZJvandeWeg ZJvandeWeg temporarily deployed to stage October 5, 2023 10:17 — with GitHub Actions Inactive
@hardillb hardillb merged commit c93cf1e into main Oct 5, 2023
14 checks passed
@hardillb hardillb deleted the zj-prom-config branch October 5, 2023 10:29
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