Add telemetry support #158

ryanartecona · 2024-03-07T23:47:43Z

This introduces a telemetry deployment and supporting resources which, when enabled, will collect metrics and logs from the other pods in the chart. When the telemetry deployment is enabled, those metrics and logs will be sent to Retool, for use cases such as proactively finding retool bugs which manifest in self-hosted deployment logs and having an easier path for Retool staff to diagnose deployment issues in a self-hosted deployment.

ryanartecona · 2024-03-08T00:50:51Z

charts/retool/templates/_helpers.tpl

+*/}}
+{{- define "retool.workflowBackend.selectorLabels" -}}
+retoolService: {{ template "retool.workflowBackend.name" . }}
+{{- end }}


This and all the retool.*.selectorLabels partials below don't actually change the selector labels that were already used, they're just refactoring the template code.

ryanartecona · 2024-03-08T00:51:28Z

charts/retool/templates/_helpers.tpl

+*/}}
+{{- define "retool.workflowWorker.name" -}}
+{{ template "retool.fullname" . }}-workflow-worker
+{{- end -}}


These result in the exact same names as before, just a template code refactor for reuse.

ryanartecona · 2024-03-08T00:56:05Z

charts/retool/templates/deployment_backend.yaml

@@ -54,7 +55,7 @@ spec:
 {{- end }}
 {{- end }}
      containers:
-      - name: {{ .Chart.Name }}
+      - name: main-backend


container names are mutable, since the pods just get recreated as normal. using semantic container names helps log collection and metric collection, since deployment name isn't always available and pod name is dependent on nameOverride and such.

👍 for the change, let's finalize the name of this service. esp. as we update docs here: https://github.com/tryretool/docs/pull/647

Candidates

main-backend

api-backend

api

backend

main-backed is nice because it corresponds to MAIN_BACKEND in SERVICE_TYPE at least.

let's go with "main-backend" even tho this has services bundled into it (including db connector)

"api" is not as discoverable in docs.

ryanartecona · 2024-03-08T00:56:22Z

charts/retool/templates/deployment_backend.yaml

@@ -244,6 +248,7 @@ spec:
            port: {{ .Values.service.internalPort }}
          initialDelaySeconds: {{ .Values.livenessProbe.initialDelaySeconds }}
          timeoutSeconds: {{ .Values.livenessProbe.timeoutSeconds }}
+          periodSeconds: {{ .Values.livenessProbe.periodSeconds }}


this was present in values.yaml but not in the template, so I assume just an oversight.

ryanartecona · 2024-03-08T00:57:30Z

charts/retool/templates/telemetry_deployment.yaml

+      annotations:
+        {{- if or .Values.telemetry.customVectorConfig .Values.telemetry.customGrafanaAgentConfig }}
+        checksum/custom-config: {{ include (print $.Template.BasePath "/telemetry_configmap.yaml") . | sha256sum | quote }}
+        {{- end }}


this makes it so that custom config changes will still roll the telemetry pod even if the pod itself doesn't change

avimoondra

🚢

avimoondra · 2024-03-12T12:25:31Z

charts/retool/templates/deployment_backend.yaml

@@ -54,7 +55,7 @@ spec:
 {{- end }}
 {{- end }}
      containers:
-      - name: {{ .Chart.Name }}
+      - name: main-backend


👍 for the change, let's finalize the name of this service. esp. as we update docs here: https://github.com/tryretool/docs/pull/647

Candidates

main-backend

api-backend

api

backend

main-backed is nice because it corresponds to MAIN_BACKEND in SERVICE_TYPE at least.

avimoondra · 2024-03-12T12:57:46Z

charts/retool/templates/_helpers.tpl

+deployment recreation and incur downtime, so should be avoided.
+*/}}
+{{- define "retool.codeExecutor.selectorLabels" -}}
+retoolService: {{ template "retool.codeExecutor.name" . }}


Suggested change

retoolService: {{ template "retool.codeExecutor.name" . }}

retoolService: {{ include "retool.codeExecutor.name" . }}

same effect, just for consistency.

avimoondra · 2024-03-12T12:58:03Z

charts/retool/templates/_helpers.tpl

+deployment recreation and incur downtime, so should be avoided.
+*/}}
+{{- define "retool.workflowWorker.selectorLabels" -}}
+retoolService: {{ template "retool.workflowWorker.name" . }}


Suggested change

retoolService: {{ template "retool.workflowWorker.name" . }}

retoolService: {{ include "retool.workflowWorker.name" . }}

avimoondra · 2024-03-12T12:58:29Z

charts/retool/templates/_helpers.tpl

+deployment recreation and incur downtime, so should be avoided.
+*/}}
+{{- define "retool.workflowBackend.selectorLabels" -}}
+retoolService: {{ template "retool.workflowBackend.name" . }}


Suggested change

retoolService: {{ template "retool.workflowBackend.name" . }}

retoolService: {{ include "retool.workflowBackend.name" . }}

same effect, just for consistency.

avimoondra · 2024-03-12T13:06:49Z

charts/retool/templates/telemetry_deployment.yaml

deployment_telemetry.yaml perhaps for consistency

avimoondra · 2024-03-12T13:07:31Z

charts/retool/templates/telemetry_deployment.yaml

+metadata:
+  name: {{ include "retool.fullname" . }}-telemetry
+  labels:
+    {{- include "retool.telemetry.labels" . | nindent 4 }}


Suggested change

{{- include "retool.telemetry.labels" . | nindent 4 }}

{{- include "retool.telemetry.labels" . | nindent 4 }}

{{- include "retool.telemetry.selectorLabels" . | nindent 4 }}

not sure if needed, but for consistency as well

avimoondra · 2024-03-12T13:07:44Z

charts/retool/templates/telemetry_deployment.yaml

+metadata:
+  name: {{ include "retool.telemetry.fullname" . }}
+  labels:
+    {{- include "retool.telemetry.labels" . | nindent 4 }}


Suggested change

{{- include "retool.telemetry.labels" . | nindent 4 }}

{{- include "retool.telemetry.labels" . | nindent 4 }}

{{- include "retool.telemetry.selectorLabels" . | nindent 4 }}

avimoondra · 2024-03-12T13:15:23Z

charts/retool/templates/telemetry_deployment.yaml

+          - name: RTEL_DEPLOYMENT_MODE
+            value: 'kubernetes-helm'
+          - name: RTEL_HELM_RELEASE_NAME
+            value: {{ .Release.Name | quote }}


maybe: align this to https://github.com/tryretool/retool_development/blob/dev/backend/src/server/modules/jobsRunner/startupTelemetry.ts#L49-L62

DEPLOYMENT_TEMPLATE_TYPE
DEPLOYMENT_TEMPLATE_VERSION

I think I will need to keep the env vars RTEL_DEPLOYMENT_MODE and DEPLOYMENT_TEMPLATE_TYPE separate after all, because I think they will diverge for other deployment templates. I'll still at least unify the values so e.g. k8s-helm is used by both, and also add DEPLOYMENT_TEMPLATE_TYPE as tags to metrics/logs when present.

ack ok 👍

avimoondra · 2024-03-12T13:22:44Z

charts/retool/templates/_helpers.tpl

@@ -44,21 +44,74 @@ Common labels
 */}}
 {{- define "retool.labels" -}}
 helm.sh/chart: {{ include "retool.chart" . }}
-{{ include "retool.selectorLabels" . }}
 {{- if .Chart.AppVersion }}
 app.kubernetes.io/version: {{ .Chart.AppVersion | quote }}


what is AppVersion vs. .Chart.Version ? (only diff from retool.telemetry.labels)

I see it's used by retool-temporal-services-helm only 👀

pretty sure .Chart.Version is the chart version, and .Chart.AppVersion would be the retool version. except .Chart.AppVersion would come from appVersion: in Chart.yaml, which is optional and absent for our chart. so I think .Chart.AppVersion is effectively always null, and we don't use it to specify retool version anyway. docs: https://helm.sh/docs/topics/charts/

I didn't realize all this when I first wrote this here though, so I'm just going to take it out so it doesn't confuse us further.

ryanartecona force-pushed the ra/telemetry branch 4 times, most recently from 667c11a to dd06dfe Compare March 8, 2024 00:48

ryanartecona requested review from avimoondra, roberto-retool and KnlnKS March 8, 2024 00:49

ryanartecona commented Mar 8, 2024

View reviewed changes

ryanartecona force-pushed the ra/telemetry branch from dd06dfe to 0eab868 Compare March 8, 2024 01:16

KnlnKS approved these changes Mar 8, 2024

View reviewed changes

avimoondra approved these changes Mar 12, 2024

View reviewed changes

ryanartecona force-pushed the ra/telemetry branch 2 times, most recently from 593bb59 to b29363a Compare March 19, 2024 23:29

ryanartecona added 9 commits April 12, 2024 16:06

Refactor workflows label selectors to support telemetry logs

6aae4ac

give pod containers semantic names for telemetry

9f295de

Add telemetry support

b6bfb3f

Bump version

d2f8077

PR feedback

3e06284

Add telemetry.extraEnv

3e08c5f

Set RTEL_ENABLED=true when telemetry container enabled

a080818

Make telemetry deployment configurable

12c35f1

Make CI kubeconform check all values files

ba0982f

ryanartecona force-pushed the ra/telemetry branch from 8ea6630 to ba0982f Compare April 12, 2024 23:55

ryanartecona added 4 commits April 12, 2024 16:58

Make default values files line up

119f8ab

Fix CI

977fff6

Better fix CI

6581b2e

Make .helmignore respected again

9afccbd

ryanartecona merged commit bc3f63a into main Apr 16, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add telemetry support #158

Add telemetry support #158

ryanartecona commented Mar 7, 2024 •

edited

Loading

ryanartecona Mar 8, 2024

ryanartecona Mar 8, 2024

ryanartecona Mar 8, 2024

avimoondra Mar 12, 2024

avimoondra Mar 15, 2024

ryanartecona Mar 8, 2024

ryanartecona Mar 8, 2024

avimoondra left a comment

avimoondra Mar 12, 2024

avimoondra Mar 12, 2024 •

edited

Loading

avimoondra Mar 12, 2024

avimoondra Mar 12, 2024

avimoondra Mar 12, 2024

avimoondra Mar 12, 2024

avimoondra Mar 12, 2024

avimoondra Mar 12, 2024

avimoondra Mar 12, 2024

ryanartecona Mar 19, 2024

avimoondra Mar 20, 2024

avimoondra Mar 12, 2024

ryanartecona Mar 19, 2024

	retoolService: {{ template "retool.codeExecutor.name" . }}
	retoolService: {{ include "retool.codeExecutor.name" . }}

	retoolService: {{ template "retool.workflowWorker.name" . }}
	retoolService: {{ include "retool.workflowWorker.name" . }}

	{{- include "retool.telemetry.labels" . \| nindent 4 }}
	{{- include "retool.telemetry.labels" . \| nindent 4 }}
	{{- include "retool.telemetry.selectorLabels" . \| nindent 4 }}

Add telemetry support #158

Add telemetry support #158

Conversation

ryanartecona commented Mar 7, 2024 • edited Loading

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

avimoondra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avimoondra Mar 12, 2024 • edited Loading

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

ryanartecona commented Mar 7, 2024 •

edited

Loading

avimoondra Mar 12, 2024 •

edited

Loading