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

Jaeger v2 with v1 #613

Merged
merged 18 commits into from
Nov 3, 2024
Merged
3 changes: 2 additions & 1 deletion .github/workflows/lint-test.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -69,4 +69,5 @@ jobs:
cmctl check api --wait=5m

- name: Run chart-testing (install)
run: ct install --config ct.yaml
run: |
ct install --config ct.yaml --helm-extra-set-args "--set provisionDataStore.cassandra=false --set storage.type=memory --set allInOne.enabled=true --set agent.enabled=false --set collector.enabled=false --set query.enabled=false"
Copy link
Member

Choose a reason for hiding this comment

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

this line is hard to read, please use the same formatting as in the deleted sections, one flag per line

4 changes: 2 additions & 2 deletions charts/jaeger/Chart.yaml
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
apiVersion: v2
appVersion: 1.53.0
appVersion: 2.0.0-rc2
description: A Jaeger Helm chart for Kubernetes
name: jaeger
type: application
version: 3.3.1
version: 3.3.2
Copy link
Member

Choose a reason for hiding this comment

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

maybe go to 4.x? It's not backwards compatible with chart-3.x which only works with jaeger-v1

# CronJobs require v1.21
kubeVersion: ">= 1.21-0"
keywords:
Expand Down
29 changes: 27 additions & 2 deletions charts/jaeger/templates/_helpers.tpl
Original file line number Diff line number Diff line change
Expand Up @@ -559,14 +559,14 @@ If not tag is provided, it defaults to .Chart.AppVersion.
{{- end -}}

{{/*
Create image name for all in one image
Create image name for all-in-one image
*/}}
{{- define "allInOne.image" -}}
{{- include "renderImage" ( dict "imageRoot" .Values.allInOne.image "context" $ ) -}}
{{- end -}}

{{/*
Create pull secrets for all in one image
Create pull secrets for all-in-one image
*/}}
{{- define "allInOne.imagePullSecrets" -}}
{{- include "common.images.renderPullSecrets" (dict "images" (list .Values.allInOne.image) "context" $) -}}
Expand Down Expand Up @@ -719,3 +719,28 @@ Create pull secrets for hotrod image
{{- define "hotrod.imagePullSecrets" -}}
{{- include "common.images.renderPullSecrets" (dict "images" (list .Values.hotrod.image) "context" $) -}}
{{- end }}


{{- define "jaeger.extensionsConfig" -}}
{{ toYaml .Values.config.extensions | nindent 6 }}
{{- end }}

{{- define "jaeger.receiversConfig" -}}
{{ toYaml .Values.config.receivers | nindent 6 }}
{{- end }}

{{- define "jaeger.processorsConfig" -}}
{{ toYaml .Values.config.processors | nindent 6 }}
{{- end }}

{{- define "jaeger.exportersConfig" -}}
{{ toYaml .Values.config.exporters | nindent 6 }}
{{- end }}

{{- define "jaeger.namespace" -}}
{{- if .Values.namespaceOverride -}}
{{- .Values.namespaceOverride -}}
{{- else -}}
{{- .Release.Namespace -}}
{{- end -}}
{{- end -}}
27 changes: 27 additions & 0 deletions charts/jaeger/templates/allInOne-config.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
{{- if .Values.allInOne.enabled -}}
yurishkuro marked this conversation as resolved.
Show resolved Hide resolved
apiVersion: v1
kind: ConfigMap
metadata:
name: jaeger-configmap
namespace: {{ include "jaeger.namespace" . }}
labels:
{{- include "jaeger.labels" . | nindent 4 }}
Copy link
Member

Choose a reason for hiding this comment

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

is this much whitespace on the left expected? Could we use normal indentation

  labels:
    {{- include "jaeger.labels" . | nindent 4 }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

could you explain what do yo mean by "normal indentation" i used this format because that is how labels are indented thought-out the chart

Copy link
Member

Choose a reason for hiding this comment

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

I showed example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh got it will fix it

data:
config.yaml: |
service:
extensions: [{{ join ", " .Values.config.service.extensions }}]
pipelines:
traces:
receivers: [{{ join ", " .Values.config.service.pipelines.traces.receivers }}]
processors: [{{ join ", " .Values.config.service.pipelines.traces.processors }}]
exporters: [{{ join ", " .Values.config.service.pipelines.traces.exporters }}]

extensions:
{{- include "jaeger.extensionsConfig" . | nindent 6 }}
receivers:
{{- include "jaeger.receiversConfig" . | nindent 6 }}
processors:
{{- include "jaeger.processorsConfig" . | nindent 6 }}
exporters:
{{- include "jaeger.exportersConfig" . | nindent 6 }}
{{- end }}
17 changes: 13 additions & 4 deletions charts/jaeger/templates/allinone-deploy.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ spec:
imagePullPolicy: {{ .Values.allInOne.image.pullPolicy }}
name: jaeger
args:
- "--config"
- "/etc/jaeger/config.yaml"
{{- range $arg := .Values.allInOne.args }}
- "{{ tpl $arg $ }}"
{{- end }}
Expand All @@ -80,11 +82,13 @@ spec:
protocol: TCP
- containerPort: 4318
protocol: TCP
- containerPort: 13133
protocol: TCP
livenessProbe:
failureThreshold: 5
httpGet:
path: /
port: 14269
path: /status
port: 13133
scheme: HTTP
initialDelaySeconds: 5
periodSeconds: 15
Expand All @@ -93,8 +97,8 @@ spec:
readinessProbe:
failureThreshold: 3
httpGet:
path: /
port: 14269
path: /status
port: 13133
scheme: HTTP
initialDelaySeconds: 1
periodSeconds: 10
Expand All @@ -105,6 +109,8 @@ spec:
{{- toYaml . | nindent 12 }}
{{- end }}
volumeMounts:
- name: jaeger-config
mountPath: /etc/jaeger
{{- if not .Values.storage.badger.ephemeral }}
- name: badger-data
mountPath: {{ .Values.storage.badger.persistence.mountPath }}
Expand All @@ -123,6 +129,9 @@ spec:
{{- toYaml .Values.allInOne.podSecurityContext | nindent 8 }}
serviceAccountName: {{ template "jaeger.fullname" . }}
volumes:
- name: jaeger-config
configMap:
name: jaeger-configmap
Copy link
Member

Choose a reason for hiding this comment

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

please clean-up all the naming. Not jaeger-config, not jaeger-configmap, not allInOne-config.yaml, not config.yaml - everything should be using a single name user-config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done should i convert it to PR

{{- if not .Values.storage.badger.ephemeral }}
- name: badger-data
persistentVolumeClaim:
Expand Down
54 changes: 53 additions & 1 deletion charts/jaeger/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,58 @@
# This is a YAML-formatted file.
# Jaeger values are grouped by component. Cassandra values override subchart values

config:
yurishkuro marked this conversation as resolved.
Show resolved Hide resolved
service:
extensions: [jaeger_storage, jaeger_query, healthcheckv2]
pipelines:
traces:
receivers: [otlp, jaeger, zipkin]
processors: [batch]
exporters: [jaeger_storage_exporter]

extensions:
healthcheckv2:
use_v2: true
http:
endpoint: 0.0.0.0:13133

# pprof:
# endpoint: 0.0.0.0:1777
# zpages:
# endpoint: 0.0.0.0:55679

jaeger_query:
storage:
traces: primary_store
traces_archive: archive_store

jaeger_storage:
backends:
primary_store:
memory:
max_traces: 100000
archive_store:
memory:
max_traces: 100000

receivers:
otlp:
protocols:
grpc:
http:
jaeger:
protocols:
grpc:
zipkin:

processors:
batch:

exporters:
jaeger_storage_exporter:
trace_storage: primary_store
# The following settings apply to Jaeger v1 and partially to Jaeger v2

global:
yurishkuro marked this conversation as resolved.
Show resolved Hide resolved
imageRegistry:

Expand All @@ -24,7 +76,7 @@ allInOne:
replicas: 1
image:
registry: ""
repository: jaegertracing/all-in-one
repository: jaegertracing/jaeger
tag: ""
digest: ""
pullPolicy: IfNotPresent
Expand Down
Loading