You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "lianneli (via GitHub)" <gi...@apache.org> on 2024/08/08 11:10:00 UTC
[PR] [CELEBORN-1552] automatically support prometheus to scrape metrics for helm chart [celeborn]
lianneli opened a new pull request, #2673:
URL: https://github.com/apache/celeborn/pull/2673
<!--
Thanks for sending a pull request! Here are some tips for you:
- Make sure the PR title start w/ a JIRA ticket, e.g. '[CELEBORN-XXXX] Your PR title ...'.
- Be sure to keep the PR description updated to reflect all changes.
- Please write your PR title to summarize what this PR proposes.
- If possible, provide a concise example to reproduce the issue for a faster review.
-->
### What changes were proposed in this pull request?
1. Add Annotations to Master Service and Worker Service for automatically scraping by Prometheus.
2. Add Ports to Worker Service, since it's empty before that prometheus cannot connect to workers.
### Why are the changes needed?
Although master and worker provide http interfaces, We still need add annotations manually for prometheus automatically scraping.
### Does this PR introduce _any_ user-facing change?
No. Users will not feel any changes to install and use Celeborn.
### How was this patch tested?
test locally and in dev environment.
Before:
After:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1552] automatically support prometheus to scrape metrics for helm chart [celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #2673:
URL: https://github.com/apache/celeborn/pull/2673#discussion_r1730633202
##########
charts/celeborn/templates/_helpers.tpl:
##########
@@ -155,3 +155,78 @@ Create the name of the worker podmonitor to use
{{- define "celeborn.workerPodMonitorName" -}}
{{ include "celeborn.fullname" . }}-worker-podmonitor
{{- end }}
+
+{{/*
+Create master annotations if metrics enables
+*/}}
+{{- define "celeborn.masterMetricsAnnotation" -}}
+{{- $metricsEnabled := true -}}
+{{- $metricsPath := "/metrics/prometheus" -}}
+{{- $masterPort := 9098 -}}
+{{- range $key, $val := .Values.celeborn }}
+{{- if eq $key "celeborn.metrics.enabled" }}
+{{- $metricsEnabled = $val -}}
+{{- end }}
+{{- if eq $key "celeborn.metrics.prometheus.path" }}
+{{- $metricsPath = $val -}}
+{{- end }}
+{{- if eq $key "celeborn.master.http.port" }}
+{{- $masterPort = $val -}}
+{{- end }}
+{{- end }}
+{{- if eq (toString $metricsEnabled) "true" -}}
+prometheus.io/path: {{ $metricsPath }}
+prometheus.io/port: '{{ $masterPort }}'
+prometheus.io/scheme: 'http'
+prometheus.io/scrape: 'true'
+{{- end }}
+{{- end }}
+
+{{/*
+Create worker annotations if metrics enables
+*/}}
+{{- define "celeborn.workerMetricsAnnotation" -}}
+{{- $metricsEnabled := true -}}
+{{- $metricsPath := "/metrics/prometheus" -}}
+{{- $workerPort := 9096 -}}
+{{- range $key, $val := .Values.celeborn }}
+{{- if eq $key "celeborn.metrics.enabled" }}
+{{- $metricsEnabled = $val -}}
+{{- end }}
+{{- if eq $key "celeborn.metrics.prometheus.path" }}
+{{- $metricsPath = $val -}}
+{{- end }}
+{{- if eq $key "celeborn.worker.http.port" }}
+{{- $workerPort = $val -}}
+{{- end }}
+{{- end }}
+{{- if eq (toString $metricsEnabled) "true" -}}
+prometheus.io/path: {{ $metricsPath }}
+prometheus.io/port: '{{ $workerPort }}'
+prometheus.io/scheme: 'http'
+prometheus.io/scrape: 'true'
+{{- end }}
+{{- end }}
+
+{{/*
Review Comment:
Sounds reasonable. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1552] automatically support prometheus to scrape metrics for helm chart [celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on PR #2673:
URL: https://github.com/apache/celeborn/pull/2673#issuecomment-2283019783
@RexXiong Could you please take a look at this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1552] automatically support prometheus to scrape metrics for helm chart [celeborn]
Posted by "lianneli (via GitHub)" <gi...@apache.org>.
lianneli commented on code in PR #2673:
URL: https://github.com/apache/celeborn/pull/2673#discussion_r1730037430
##########
charts/celeborn/templates/_helpers.tpl:
##########
@@ -155,3 +155,78 @@ Create the name of the worker podmonitor to use
{{- define "celeborn.workerPodMonitorName" -}}
{{ include "celeborn.fullname" . }}-worker-podmonitor
{{- end }}
+
+{{/*
+Create master annotations if metrics enables
+*/}}
+{{- define "celeborn.masterMetricsAnnotation" -}}
+{{- $metricsEnabled := true -}}
+{{- $metricsPath := "/metrics/prometheus" -}}
+{{- $masterPort := 9098 -}}
+{{- range $key, $val := .Values.celeborn }}
+{{- if eq $key "celeborn.metrics.enabled" }}
+{{- $metricsEnabled = $val -}}
+{{- end }}
+{{- if eq $key "celeborn.metrics.prometheus.path" }}
+{{- $metricsPath = $val -}}
+{{- end }}
+{{- if eq $key "celeborn.master.http.port" }}
+{{- $masterPort = $val -}}
+{{- end }}
+{{- end }}
+{{- if eq (toString $metricsEnabled) "true" -}}
+prometheus.io/path: {{ $metricsPath }}
+prometheus.io/port: '{{ $masterPort }}'
+prometheus.io/scheme: 'http'
+prometheus.io/scrape: 'true'
+{{- end }}
+{{- end }}
+
+{{/*
+Create worker annotations if metrics enables
+*/}}
+{{- define "celeborn.workerMetricsAnnotation" -}}
+{{- $metricsEnabled := true -}}
+{{- $metricsPath := "/metrics/prometheus" -}}
+{{- $workerPort := 9096 -}}
+{{- range $key, $val := .Values.celeborn }}
+{{- if eq $key "celeborn.metrics.enabled" }}
+{{- $metricsEnabled = $val -}}
+{{- end }}
+{{- if eq $key "celeborn.metrics.prometheus.path" }}
+{{- $metricsPath = $val -}}
+{{- end }}
+{{- if eq $key "celeborn.worker.http.port" }}
+{{- $workerPort = $val -}}
+{{- end }}
+{{- end }}
+{{- if eq (toString $metricsEnabled) "true" -}}
+prometheus.io/path: {{ $metricsPath }}
+prometheus.io/port: '{{ $workerPort }}'
+prometheus.io/scheme: 'http'
+prometheus.io/scrape: 'true'
+{{- end }}
+{{- end }}
+
+{{/*
Review Comment:
> > Of Course, we can add the prometheus http port in master service for understanding more easily. Should I do that?
>
> No, but I'm still very interested in why Prometheus can still scrape metrics successfully.
@RexXiong
Good question, and I made a test.
In k8s, the core role of Service is translating **Pod's ip** to domain name like "xxx.namespace.svc.cluster.local", whether the port set in Service is a real one. However, Service will not work if there is none port set.
Prometheus would search all the "on working" service, fetch annotations to get real port, then get **Pod's ip** though K8s Api to finish the metrics scratching.
That's why we need not set another port in Master Service.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1552] automatically support prometheus to scrape metrics for helm chart [celeborn]
Posted by "FMX (via GitHub)" <gi...@apache.org>.
FMX commented on code in PR #2673:
URL: https://github.com/apache/celeborn/pull/2673#discussion_r1713129184
##########
charts/celeborn/templates/_helpers.tpl:
##########
@@ -155,3 +155,78 @@ Create the name of the worker podmonitor to use
{{- define "celeborn.workerPodMonitorName" -}}
{{ include "celeborn.fullname" . }}-worker-podmonitor
{{- end }}
+
+{{/*
+Create master annotations if metrics enables
+*/}}
+{{- define "celeborn.masterMetricsAnnotation" -}}
+{{- $metricsEnabled := true -}}
+{{- $metricsPath := "/metrics/prometheus" -}}
+{{- $masterPort := 9098 -}}
+{{- range $key, $val := .Values.celeborn }}
+{{- if eq $key "celeborn.metrics.enabled" }}
+{{- $metricsEnabled = $val -}}
+{{- end }}
Review Comment:
I've a question about helm chart indentation? Is there any guide line or best practice?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1552] automatically support prometheus to scrape metrics for helm chart [celeborn]
Posted by "lianneli (via GitHub)" <gi...@apache.org>.
lianneli commented on code in PR #2673:
URL: https://github.com/apache/celeborn/pull/2673#discussion_r1715011239
##########
charts/celeborn/templates/_helpers.tpl:
##########
@@ -155,3 +155,78 @@ Create the name of the worker podmonitor to use
{{- define "celeborn.workerPodMonitorName" -}}
{{ include "celeborn.fullname" . }}-worker-podmonitor
{{- end }}
+
+{{/*
+Create master annotations if metrics enables
+*/}}
+{{- define "celeborn.masterMetricsAnnotation" -}}
+{{- $metricsEnabled := true -}}
+{{- $metricsPath := "/metrics/prometheus" -}}
+{{- $masterPort := 9098 -}}
+{{- range $key, $val := .Values.celeborn }}
+{{- if eq $key "celeborn.metrics.enabled" }}
+{{- $metricsEnabled = $val -}}
+{{- end }}
Review Comment:
Thanks for you review. The file `_helpers.tpl` is go template syntax, some complex logic can be coded in this file, I understand. The param `celeborn.metrics.enabled`, and others, is written in values.yaml as a single key, but the dot `.` inside key does not conform to the specification which cannot be used directly by `.Values.xxx`. So, I imitate the way in `configMap.yaml`, using the `range` syntax to loop through, and using if to scratch the value. The indentation and format in this file is not significant, and it is processed through variable in template/xx/service.yaml.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1552] automatically support prometheus to scrape metrics for helm chart [celeborn]
Posted by "lianneli (via GitHub)" <gi...@apache.org>.
lianneli commented on code in PR #2673:
URL: https://github.com/apache/celeborn/pull/2673#discussion_r1716377276
##########
charts/celeborn/templates/_helpers.tpl:
##########
@@ -155,3 +155,78 @@ Create the name of the worker podmonitor to use
{{- define "celeborn.workerPodMonitorName" -}}
{{ include "celeborn.fullname" . }}-worker-podmonitor
{{- end }}
+
+{{/*
+Create master annotations if metrics enables
+*/}}
+{{- define "celeborn.masterMetricsAnnotation" -}}
+{{- $metricsEnabled := true -}}
+{{- $metricsPath := "/metrics/prometheus" -}}
+{{- $masterPort := 9098 -}}
+{{- range $key, $val := .Values.celeborn }}
+{{- if eq $key "celeborn.metrics.enabled" }}
+{{- $metricsEnabled = $val -}}
+{{- end }}
Review Comment:
> I have a question about helm chart indentation. Is there any guideline or best practice?
sorry for not link your review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1552] automatically support prometheus to scrape metrics for helm chart [celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #2673:
URL: https://github.com/apache/celeborn/pull/2673#discussion_r1721195705
##########
charts/celeborn/templates/_helpers.tpl:
##########
@@ -155,3 +155,78 @@ Create the name of the worker podmonitor to use
{{- define "celeborn.workerPodMonitorName" -}}
{{ include "celeborn.fullname" . }}-worker-podmonitor
{{- end }}
+
+{{/*
+Create master annotations if metrics enables
+*/}}
+{{- define "celeborn.masterMetricsAnnotation" -}}
+{{- $metricsEnabled := true -}}
+{{- $metricsPath := "/metrics/prometheus" -}}
+{{- $masterPort := 9098 -}}
+{{- range $key, $val := .Values.celeborn }}
+{{- if eq $key "celeborn.metrics.enabled" }}
+{{- $metricsEnabled = $val -}}
+{{- end }}
+{{- if eq $key "celeborn.metrics.prometheus.path" }}
+{{- $metricsPath = $val -}}
+{{- end }}
+{{- if eq $key "celeborn.master.http.port" }}
+{{- $masterPort = $val -}}
+{{- end }}
+{{- end }}
+{{- if eq (toString $metricsEnabled) "true" -}}
+prometheus.io/path: {{ $metricsPath }}
+prometheus.io/port: '{{ $masterPort }}'
+prometheus.io/scheme: 'http'
+prometheus.io/scrape: 'true'
+{{- end }}
+{{- end }}
+
+{{/*
+Create worker annotations if metrics enables
+*/}}
+{{- define "celeborn.workerMetricsAnnotation" -}}
+{{- $metricsEnabled := true -}}
+{{- $metricsPath := "/metrics/prometheus" -}}
+{{- $workerPort := 9096 -}}
+{{- range $key, $val := .Values.celeborn }}
+{{- if eq $key "celeborn.metrics.enabled" }}
+{{- $metricsEnabled = $val -}}
+{{- end }}
+{{- if eq $key "celeborn.metrics.prometheus.path" }}
+{{- $metricsPath = $val -}}
+{{- end }}
+{{- if eq $key "celeborn.worker.http.port" }}
+{{- $workerPort = $val -}}
+{{- end }}
+{{- end }}
+{{- if eq (toString $metricsEnabled) "true" -}}
+prometheus.io/path: {{ $metricsPath }}
+prometheus.io/port: '{{ $workerPort }}'
+prometheus.io/scheme: 'http'
+prometheus.io/scrape: 'true'
+{{- end }}
+{{- end }}
+
+{{/*
Review Comment:
Is this also required for Celeborn Master?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1552] automatically support prometheus to scrape metrics for helm chart [celeborn]
Posted by "lianneli (via GitHub)" <gi...@apache.org>.
lianneli commented on code in PR #2673:
URL: https://github.com/apache/celeborn/pull/2673#discussion_r1723095199
##########
charts/celeborn/templates/_helpers.tpl:
##########
@@ -155,3 +155,78 @@ Create the name of the worker podmonitor to use
{{- define "celeborn.workerPodMonitorName" -}}
{{ include "celeborn.fullname" . }}-worker-podmonitor
{{- end }}
+
+{{/*
+Create master annotations if metrics enables
+*/}}
+{{- define "celeborn.masterMetricsAnnotation" -}}
+{{- $metricsEnabled := true -}}
+{{- $metricsPath := "/metrics/prometheus" -}}
+{{- $masterPort := 9098 -}}
+{{- range $key, $val := .Values.celeborn }}
+{{- if eq $key "celeborn.metrics.enabled" }}
+{{- $metricsEnabled = $val -}}
+{{- end }}
+{{- if eq $key "celeborn.metrics.prometheus.path" }}
+{{- $metricsPath = $val -}}
+{{- end }}
+{{- if eq $key "celeborn.master.http.port" }}
+{{- $masterPort = $val -}}
+{{- end }}
+{{- end }}
+{{- if eq (toString $metricsEnabled) "true" -}}
+prometheus.io/path: {{ $metricsPath }}
+prometheus.io/port: '{{ $masterPort }}'
+prometheus.io/scheme: 'http'
+prometheus.io/scrape: 'true'
+{{- end }}
+{{- end }}
+
+{{/*
+Create worker annotations if metrics enables
+*/}}
+{{- define "celeborn.workerMetricsAnnotation" -}}
+{{- $metricsEnabled := true -}}
+{{- $metricsPath := "/metrics/prometheus" -}}
+{{- $workerPort := 9096 -}}
+{{- range $key, $val := .Values.celeborn }}
+{{- if eq $key "celeborn.metrics.enabled" }}
+{{- $metricsEnabled = $val -}}
+{{- end }}
+{{- if eq $key "celeborn.metrics.prometheus.path" }}
+{{- $metricsPath = $val -}}
+{{- end }}
+{{- if eq $key "celeborn.worker.http.port" }}
+{{- $workerPort = $val -}}
+{{- end }}
+{{- end }}
+{{- if eq (toString $metricsEnabled) "true" -}}
+prometheus.io/path: {{ $metricsPath }}
+prometheus.io/port: '{{ $workerPort }}'
+prometheus.io/scheme: 'http'
+prometheus.io/scrape: 'true'
+{{- end }}
+{{- end }}
+
+{{/*
Review Comment:
> Is this also required for Celeborn Master?
Not required. Something interesting is although the port set in master service is not for metrics, prometheus still can scrape metrics successfully. This is not proper for worker service, since there is not any port set in it. So I only set the prometheus http port in worker service other than master worker. All the changes have tested in our clusters, containing both dev and prod environment.
Of Course, we can add the prometheus http port in master service for understanding more easily. Should I do that?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1552] automatically support prometheus to scrape metrics for helm chart [celeborn]
Posted by "lianneli (via GitHub)" <gi...@apache.org>.
lianneli commented on code in PR #2673:
URL: https://github.com/apache/celeborn/pull/2673#discussion_r1715011239
##########
charts/celeborn/templates/_helpers.tpl:
##########
@@ -155,3 +155,78 @@ Create the name of the worker podmonitor to use
{{- define "celeborn.workerPodMonitorName" -}}
{{ include "celeborn.fullname" . }}-worker-podmonitor
{{- end }}
+
+{{/*
+Create master annotations if metrics enables
+*/}}
+{{- define "celeborn.masterMetricsAnnotation" -}}
+{{- $metricsEnabled := true -}}
+{{- $metricsPath := "/metrics/prometheus" -}}
+{{- $masterPort := 9098 -}}
+{{- range $key, $val := .Values.celeborn }}
+{{- if eq $key "celeborn.metrics.enabled" }}
+{{- $metricsEnabled = $val -}}
+{{- end }}
Review Comment:
@FMX Thanks for you review. The file `_helpers.tpl` is go template syntax, some complex logic can be coded in this file, I understand. The param `celeborn.metrics.enabled`, and others, is written in values.yaml as a single key, but the dot `.` inside key does not conform to the specification which cannot be used directly by `.Values.xxx`. So, I imitate the way in `configMap.yaml`, using the `range` syntax to loop through, and using if to scratch the value. The indentation and format in this file is not significant, and it is processed through variable in template/xx/service.yaml.
##########
charts/celeborn/templates/_helpers.tpl:
##########
@@ -155,3 +155,78 @@ Create the name of the worker podmonitor to use
{{- define "celeborn.workerPodMonitorName" -}}
{{ include "celeborn.fullname" . }}-worker-podmonitor
{{- end }}
+
+{{/*
+Create master annotations if metrics enables
+*/}}
+{{- define "celeborn.masterMetricsAnnotation" -}}
+{{- $metricsEnabled := true -}}
+{{- $metricsPath := "/metrics/prometheus" -}}
+{{- $masterPort := 9098 -}}
+{{- range $key, $val := .Values.celeborn }}
+{{- if eq $key "celeborn.metrics.enabled" }}
+{{- $metricsEnabled = $val -}}
+{{- end }}
Review Comment:
> I have a question about helm chart indentation. Is there any guideline or best practice?
sorry for not link your review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1552] automatically support prometheus to scrape metrics for helm chart [celeborn]
Posted by "lianneli (via GitHub)" <gi...@apache.org>.
lianneli commented on code in PR #2673:
URL: https://github.com/apache/celeborn/pull/2673#discussion_r1723095199
##########
charts/celeborn/templates/_helpers.tpl:
##########
@@ -155,3 +155,78 @@ Create the name of the worker podmonitor to use
{{- define "celeborn.workerPodMonitorName" -}}
{{ include "celeborn.fullname" . }}-worker-podmonitor
{{- end }}
+
+{{/*
+Create master annotations if metrics enables
+*/}}
+{{- define "celeborn.masterMetricsAnnotation" -}}
+{{- $metricsEnabled := true -}}
+{{- $metricsPath := "/metrics/prometheus" -}}
+{{- $masterPort := 9098 -}}
+{{- range $key, $val := .Values.celeborn }}
+{{- if eq $key "celeborn.metrics.enabled" }}
+{{- $metricsEnabled = $val -}}
+{{- end }}
+{{- if eq $key "celeborn.metrics.prometheus.path" }}
+{{- $metricsPath = $val -}}
+{{- end }}
+{{- if eq $key "celeborn.master.http.port" }}
+{{- $masterPort = $val -}}
+{{- end }}
+{{- end }}
+{{- if eq (toString $metricsEnabled) "true" -}}
+prometheus.io/path: {{ $metricsPath }}
+prometheus.io/port: '{{ $masterPort }}'
+prometheus.io/scheme: 'http'
+prometheus.io/scrape: 'true'
+{{- end }}
+{{- end }}
+
+{{/*
+Create worker annotations if metrics enables
+*/}}
+{{- define "celeborn.workerMetricsAnnotation" -}}
+{{- $metricsEnabled := true -}}
+{{- $metricsPath := "/metrics/prometheus" -}}
+{{- $workerPort := 9096 -}}
+{{- range $key, $val := .Values.celeborn }}
+{{- if eq $key "celeborn.metrics.enabled" }}
+{{- $metricsEnabled = $val -}}
+{{- end }}
+{{- if eq $key "celeborn.metrics.prometheus.path" }}
+{{- $metricsPath = $val -}}
+{{- end }}
+{{- if eq $key "celeborn.worker.http.port" }}
+{{- $workerPort = $val -}}
+{{- end }}
+{{- end }}
+{{- if eq (toString $metricsEnabled) "true" -}}
+prometheus.io/path: {{ $metricsPath }}
+prometheus.io/port: '{{ $workerPort }}'
+prometheus.io/scheme: 'http'
+prometheus.io/scrape: 'true'
+{{- end }}
+{{- end }}
+
+{{/*
Review Comment:
> Is this also required for Celeborn Master?
@RexXiong Not required. Something interesting is although the port set in master service is not for metrics, prometheus still can scrape metrics successfully. This is not proper for worker service, since there is not any port set in it. So I only set the prometheus http port in worker service other than master worker. All the changes have tested in our clusters, containing both dev and prod environment.
Of Course, we can add the prometheus http port in master service for understanding more easily. Should I do that?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1552] automatically support prometheus to scrape metrics for helm chart [celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #2673:
URL: https://github.com/apache/celeborn/pull/2673#discussion_r1723530634
##########
charts/celeborn/templates/_helpers.tpl:
##########
@@ -155,3 +155,78 @@ Create the name of the worker podmonitor to use
{{- define "celeborn.workerPodMonitorName" -}}
{{ include "celeborn.fullname" . }}-worker-podmonitor
{{- end }}
+
+{{/*
+Create master annotations if metrics enables
+*/}}
+{{- define "celeborn.masterMetricsAnnotation" -}}
+{{- $metricsEnabled := true -}}
+{{- $metricsPath := "/metrics/prometheus" -}}
+{{- $masterPort := 9098 -}}
+{{- range $key, $val := .Values.celeborn }}
+{{- if eq $key "celeborn.metrics.enabled" }}
+{{- $metricsEnabled = $val -}}
+{{- end }}
+{{- if eq $key "celeborn.metrics.prometheus.path" }}
+{{- $metricsPath = $val -}}
+{{- end }}
+{{- if eq $key "celeborn.master.http.port" }}
+{{- $masterPort = $val -}}
+{{- end }}
+{{- end }}
+{{- if eq (toString $metricsEnabled) "true" -}}
+prometheus.io/path: {{ $metricsPath }}
+prometheus.io/port: '{{ $masterPort }}'
+prometheus.io/scheme: 'http'
+prometheus.io/scrape: 'true'
+{{- end }}
+{{- end }}
+
+{{/*
+Create worker annotations if metrics enables
+*/}}
+{{- define "celeborn.workerMetricsAnnotation" -}}
+{{- $metricsEnabled := true -}}
+{{- $metricsPath := "/metrics/prometheus" -}}
+{{- $workerPort := 9096 -}}
+{{- range $key, $val := .Values.celeborn }}
+{{- if eq $key "celeborn.metrics.enabled" }}
+{{- $metricsEnabled = $val -}}
+{{- end }}
+{{- if eq $key "celeborn.metrics.prometheus.path" }}
+{{- $metricsPath = $val -}}
+{{- end }}
+{{- if eq $key "celeborn.worker.http.port" }}
+{{- $workerPort = $val -}}
+{{- end }}
+{{- end }}
+{{- if eq (toString $metricsEnabled) "true" -}}
+prometheus.io/path: {{ $metricsPath }}
+prometheus.io/port: '{{ $workerPort }}'
+prometheus.io/scheme: 'http'
+prometheus.io/scrape: 'true'
+{{- end }}
+{{- end }}
+
+{{/*
Review Comment:
> Of Course, we can add the prometheus http port in master service for understanding more easily. Should I do that?
No, but I'm still very interested in why Prometheus can still scrape metrics successfully.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1552] automatically support prometheus to scrape metrics for helm chart [celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong closed pull request #2673: [CELEBORN-1552] automatically support prometheus to scrape metrics for helm chart
URL: https://github.com/apache/celeborn/pull/2673
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1552] automatically support prometheus to scrape metrics for helm chart [celeborn]
Posted by "FMX (via GitHub)" <gi...@apache.org>.
FMX commented on code in PR #2673:
URL: https://github.com/apache/celeborn/pull/2673#discussion_r1713129185
##########
charts/celeborn/templates/_helpers.tpl:
##########
@@ -155,3 +155,78 @@ Create the name of the worker podmonitor to use
{{- define "celeborn.workerPodMonitorName" -}}
{{ include "celeborn.fullname" . }}-worker-podmonitor
{{- end }}
+
+{{/*
+Create master annotations if metrics enables
+*/}}
+{{- define "celeborn.masterMetricsAnnotation" -}}
+{{- $metricsEnabled := true -}}
+{{- $metricsPath := "/metrics/prometheus" -}}
+{{- $masterPort := 9098 -}}
+{{- range $key, $val := .Values.celeborn }}
+{{- if eq $key "celeborn.metrics.enabled" }}
+{{- $metricsEnabled = $val -}}
+{{- end }}
Review Comment:
I have a question about helm chart indentation. Is there any guideline or best practice?
##########
charts/celeborn/templates/_helpers.tpl:
##########
@@ -155,3 +155,78 @@ Create the name of the worker podmonitor to use
{{- define "celeborn.workerPodMonitorName" -}}
{{ include "celeborn.fullname" . }}-worker-podmonitor
{{- end }}
+
+{{/*
+Create master annotations if metrics enables
+*/}}
+{{- define "celeborn.masterMetricsAnnotation" -}}
+{{- $metricsEnabled := true -}}
+{{- $metricsPath := "/metrics/prometheus" -}}
+{{- $masterPort := 9098 -}}
+{{- range $key, $val := .Values.celeborn }}
+{{- if eq $key "celeborn.metrics.enabled" }}
+{{- $metricsEnabled = $val -}}
+{{- end }}
Review Comment:
I've a question about helm chart indentation? Is there any guide line or best practice?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org