feat: Add support for metrics in chart #924

coibib · 2024-07-04T13:24:54Z

This PR adds a specific port for the metrics in the Helm Chart.

Signed-off-by: François BIBRON <[email protected]>

CLAassistant · 2024-07-04T13:24:58Z

All committers have signed the CLA.

xvilo · 2024-07-04T14:00:59Z

Do we also want to add support for prometheus service monitor? https://observability.thomasriley.co.uk/prometheus/configuring-prometheus/using-service-monitors/

In this case it would be automatically be picked up by prometheus-operator running on my cluster :D

Signed-off-by: François BIBRON <[email protected]>

coibib · 2024-07-04T14:28:32Z

Do we also want to add support for prometheus service monitor? https://observability.thomasriley.co.uk/prometheus/configuring-prometheus/using-service-monitors/

In this case it would be automatically be picked up by prometheus-operator running on my cluster :D

I totally agree with you, I added the corresponding changes.

Signed-off-by: François BIBRON <[email protected]>

xvilo · 2024-07-12T09:20:14Z

LGTM

dunglas

Thanks for working on this, and sorry for the late review.

I made some small suggestions inspired by https://github.com/bitnami/charts/blob/main/bitnami/wordpress/.

dunglas · 2024-08-14T18:18:39Z

charts/mercure/templates/service.yaml

+      name: metrics
+  selector:
+    {{- include "mercure.selectorLabels" . | nindent 4 }}
+{{- end }}


Suggested change

{{- end }}

{{- end }}

dunglas · 2024-08-14T18:39:30Z

charts/mercure/values.yaml

+  # admin :9181
+  enabled: false
+  # -- The port to use for exposing the metrics.
+  port: 9181


Suggested change

port: 9181

port: 2019

Can we stick with the default Caddy port?

dunglas · 2024-08-14T18:42:26Z

charts/mercure/values.yaml

@@ -28,6 +28,22 @@ subscriberJwtAlg: HS256
 # -- The license key for [the High Availability version](https://mercure.rocks/docs/hub/cluster) (not necessary is you use the FOSS version).
 license: ""

+metrics:
+  # -- Enable the metrics. You must also add the [metrics](https://caddyserver.com/docs/caddyfile/directives/metrics) in the `globalOptions` for Caddy server.


Suggested change

# -- Enable the metrics. You must also add the [metrics](https://caddyserver.com/docs/caddyfile/directives/metrics) in the `globalOptions` for Caddy server.

# -- Enable metrics. You must also add a `servers` block with a [`metrics` directive](https://caddyserver.com/docs/caddyfile/options#metrics) in the `globalOptions` value.

dunglas · 2024-08-14T18:42:54Z

charts/mercure/values.yaml

+  # servers {
+  #     metrics
+  # }
+  # admin :9181


Suggested change

# admin :9181

dunglas · 2024-08-14T19:33:06Z

charts/mercure/templates/service.yaml

+  name: {{ include "mercure.fullname" . }}-metrics
+  labels:
+    {{- include "mercure.labels" . | nindent 4 }}
+    prometheus-operated: true


Suggested change

prometheus-operated: true

prometheus-operated: true

app.kubernetes.io/component: metrics

dunglas · 2024-08-14T19:34:18Z

charts/mercure/templates/service.yaml

+  name: {{ include "mercure.fullname" . }}-metrics
+  labels:
+    {{- include "mercure.labels" . | nindent 4 }}
+    prometheus-operated: true


Why is this label necessary? I don't find where it is documented.

dunglas · 2024-08-14T19:36:58Z

charts/mercure/templates/serviceMonitor.yaml

+  labels:
+    app.kubernetes.io/name: {{ include "mercure.name" . }}


Suggested change

labels:

app.kubernetes.io/name: {{ include "mercure.name" . }}

labels:

{{- include "mercure.labels" . | nindent 4 }}

app.kubernetes.io/component: metrics

dunglas · 2024-08-14T19:37:08Z

charts/mercure/templates/serviceMonitor.yaml

+spec:
+  selector:
+    matchLabels:
+      prometheus-operated: "true"


Suggested change

prometheus-operated: "true"

app.kubernetes.io/component: metrics

charts/mercure/templates/serviceMonitor.yaml

dunglas · 2024-08-14T19:40:38Z

charts/mercure/values.yaml

@@ -28,6 +28,22 @@ subscriberJwtAlg: HS256
 # -- The license key for [the High Availability version](https://mercure.rocks/docs/hub/cluster) (not necessary is you use the FOSS version).
 license: ""

+metrics:
+  # -- Enable the metrics. You must also add the [metrics](https://caddyserver.com/docs/caddyfile/directives/metrics) in the `globalOptions` for Caddy server.
+  # servers {


Can't we add this option automatically using the by appending the needed config to the GLOBAL_OPTIONS environment variable in deployment.yaml?

Appending server {metrics} option to GLOBAL_OPTIONS may cause errors if the server option is specified elsewhere, for example in globalOptions I think

Indeed, let's do that like this for now. We could do something smarter in the future (like detection if servers is not part of the existing string, but we'll see in follow-up PRs!

Signed-off-by: François Bibron <[email protected]>

gabrielrosset · 2024-08-28T13:58:55Z

Hello @dunglas , it seems that @coibib has taken your remarks into account, is there anything left to do on this PR to merge it?

Hello @coibib , can you please allow me to push on your fork to allow us to get the subject back into the project ? Thanks.

dunglas · 2024-08-30T20:32:33Z

charts/mercure/values.yaml

@@ -28,6 +28,22 @@ subscriberJwtAlg: HS256
 # -- The license key for [the High Availability version](https://mercure.rocks/docs/hub/cluster) (not necessary is you use the FOSS version).
 license: ""

+metrics:
+  # -- Enable the metrics. You must also add the [metrics](https://caddyserver.com/docs/caddyfile/directives/metrics) in the `globalOptions` for Caddy server.
+  # servers {


Indeed, let's do that like this for now. We could do something smarter in the future (like detection if servers is not part of the existing string, but we'll see in follow-up PRs!

dunglas · 2024-08-30T20:33:23Z

charts/mercure/values.yaml

+resources:
+  {}


Why this change?

This is my linter, i reverted the change back.

Signed-off-by: François Bibron <[email protected]>

xvilo · 2024-09-09T11:26:50Z

Hiya, whats the status of this PR? We've been using Mercure in production since a month or so, and running it through kubernetes. Unfortunately we've had issues with it borking twice now, and with just the ingress logs we don't have enough data :-) so this might be super helpful 👍

dunglas · 2024-09-09T11:36:01Z

Thanks @coibib!

dunglas · 2024-09-09T11:36:25Z

@xvilo merged, it will be in the next release.

Add support for metrics in chart

4892b53

Signed-off-by: François BIBRON <[email protected]>

coibib added 2 commits July 4, 2024 16:23

feat: add serviceMonitor

e95f29d

Signed-off-by: François BIBRON <[email protected]>

feat: fix values

55c6315

Signed-off-by: François BIBRON <[email protected]>

coibib added 2 commits July 4, 2024 16:30

feat: fix values

286477e

Signed-off-by: François BIBRON <[email protected]>

feat: fix values

731a9d5

Signed-off-by: François BIBRON <[email protected]>

xvilo approved these changes Jul 12, 2024

View reviewed changes

dunglas requested changes Aug 14, 2024

View reviewed changes

fix values and typos

55accb3

Signed-off-by: François Bibron <[email protected]>

dunglas approved these changes Aug 30, 2024

View reviewed changes

fix typo

839a88e

Signed-off-by: François Bibron <[email protected]>

fix CS

4155de4

dunglas approved these changes Sep 9, 2024

View reviewed changes

dunglas merged commit 849c7b3 into dunglas:main Sep 9, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add support for metrics in chart #924

feat: Add support for metrics in chart #924

coibib commented Jul 4, 2024 •

edited

Loading

CLAassistant commented Jul 4, 2024 •

edited

Loading

xvilo commented Jul 4, 2024

coibib commented Jul 4, 2024

xvilo commented Jul 12, 2024

dunglas left a comment

dunglas Aug 14, 2024

dunglas Aug 14, 2024

dunglas Aug 14, 2024

dunglas Aug 14, 2024

dunglas Aug 14, 2024

dunglas Aug 14, 2024

dunglas Aug 14, 2024

dunglas Aug 14, 2024

dunglas Aug 14, 2024

coibib Aug 28, 2024

dunglas Aug 30, 2024

gabrielrosset commented Aug 28, 2024

dunglas Aug 30, 2024

dunglas Aug 30, 2024

coibib Sep 3, 2024

xvilo commented Sep 9, 2024

dunglas commented Sep 9, 2024

dunglas commented Sep 9, 2024

	# -- Enable the metrics. You must also add the [metrics](https://caddyserver.com/docs/caddyfile/directives/metrics) in the `globalOptions` for Caddy server.
	# -- Enable metrics. You must also add a `servers` block with a [`metrics` directive](https://caddyserver.com/docs/caddyfile/options#metrics) in the `globalOptions` value.

	prometheus-operated: true
	prometheus-operated: true
	app.kubernetes.io/component: metrics

		labels:
		app.kubernetes.io/name: {{ include "mercure.name" . }}

	prometheus-operated: "true"
	app.kubernetes.io/component: metrics

		resources:
		{}

feat: Add support for metrics in chart #924

feat: Add support for metrics in chart #924

Conversation

coibib commented Jul 4, 2024 • edited Loading

CLAassistant commented Jul 4, 2024 • edited Loading

xvilo commented Jul 4, 2024

coibib commented Jul 4, 2024

xvilo commented Jul 12, 2024

dunglas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielrosset commented Aug 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xvilo commented Sep 9, 2024

dunglas commented Sep 9, 2024

dunglas commented Sep 9, 2024

coibib commented Jul 4, 2024 •

edited

Loading

CLAassistant commented Jul 4, 2024 •

edited

Loading