-
Notifications
You must be signed in to change notification settings - Fork 673
helm: Replace internal agent with Grafana Agent #2304
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
Conversation
This reverts commit d434dcf.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
{{- range $profileTypes }} | ||
|
||
discovery.relabel "kubernetes_pods_{{.}}_default_name" { | ||
targets = concat(discovery.relabel.kubernetes_pods.output) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth leaving a comment why those need to be tabs, vs. the ones in lines 36-65 which are spaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is because of the river linter.
{{- if hasKey .Values.pyroscope.components "query-frontend" }} | ||
kubectl --namespace {{ .Release.Namespace }} port-forward svc/{{ include "pyroscope.fullname" . }}-query-frontend {{ .Values.pyroscope.service.port }}:{{ .Values.pyroscope.service.port }} | ||
http://{{ include "pyroscope.fullname" . }}-query-frontend.{{ .Release.Namespace }}.svc.cluster.local.:{{ .Values.pyroscope.service.port }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This port is wrong when you override the port only for that one component.
You would need to do the default thing here, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah not easy with the dash. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{{ $port := ((index .Values.pyroscope.components "query-frontend").service).port | default .Values.pyroscope.service.port }}
did the trick :mindblowing:
This reverts commit 4a73dee.
This replace the old agent module with Grafana Agent (as a subchart) in helm.
I decided to keep the agent on by default as it was before. I suspect this whole logic might be better in the agent helm chart.
I still needs to verify if the documentation needs edit.