Add all-in-one deployment and configmap for jaeger-v2 #606

hellspawn679 · 2024-10-01T15:51:36Z

What this PR does

Which issue this PR fixes

is a part of #610

fixes #

Checklist

DCO signed
Commits are GPG signed
Chart Version bumped
Title of the PR starts with chart name ([jaeger] or [jaeger-operator])
README.md has been updated to match version/contain new values

Signed-off-by: Mehul <mehulsharma4786@gmail.com>

charts/jaeger-v2/Chart.yaml

charts/jaeger-v2/readme.md

yurishkuro · 2024-10-01T17:05:45Z

charts/jaeger-v2/values.yaml

+      adaptive:
+        sampling_store: some_store
+        initial_sampling_probability: 0.1


I don't think we want adaptive sampling to be the default.

i tried removing it but it gives me this error

Error: invalid configuration: extensions::remote_sampling: no sampling strategy provider specified, expecting 'adaptive' or 'file' 2024/10/11 11:48:44 invalid configuration: extensions::remote_sampling: no sampling strategy provider specified, expecting 'adaptive' or 'file'```

remove remote sampling extension altogether to start with, let's get a real storage working first.

Signed-off-by: Mehul <mehulsharma4786@gmail.com>

Jaeger v2 test

Signed-off-by: mehul <mehulsharam4786@gmail.com>

Jaeger v2 test2

Signed-off-by: mehul <mehulsharam4786@gmail.com>

charts/jaeger-v2/Chart.lock

yurishkuro · 2024-10-11T15:35:32Z

charts/jaeger-v2/Chart.yaml

+kubeVersion: ">= 1.21-0"
+keywords:
+  - jaeger
+  - opentracing


Suggested change

- opentracing

- opentelemetry

Signed-off-by: mehul <mehulsharam4786@gmail.com>

Jaeger v2 test2

Signed-off-by: mehul <mehulsharam4786@gmail.com>

charts/jaeger-v2/templates/config-map.yaml

yurishkuro · 2024-10-11T19:01:04Z

charts/jaeger-v2/templates/deployment.yaml

+    spec:
+      containers:
+        - name: jaeger
+          image: jaegertracing/jaeger:latest


please use concrete version, not latest.

yurishkuro · 2024-10-11T19:01:37Z

charts/jaeger-v2/templates/deployment.yaml

+          env:
+            - name: COLLECTOR_ZIPKIN_HOST_PORT
+              value: ":9411"


This has no meaning for v2

Suggested change

env:

- name: COLLECTOR_ZIPKIN_HOST_PORT

value: ":9411"

should i also remove

- containerPort: 9411 name: zipkin

no, port mapping can stay, we still run zipkin receiver, it just doesn't need any env vars

yurishkuro · 2024-10-11T19:02:35Z

charts/jaeger-v2/templates/deployment.yaml

+      volumes:
+        - name: config-volume
+          configMap:
+            name: my-config


let's give it a proper name

how about this

volumeMounts: - name: jaeger-v2-config mountPath: /etc/jaeger volumes: - name: jaeger-v2-config configMap: name: config

yurishkuro · 2024-10-11T19:02:51Z

charts/jaeger-v2/values.yaml

+    trace_storage: some_store
+
+
+image: "jaegertracing/jaeger:latest"


yes i have fixed it

yurishkuro · 2024-10-11T19:03:37Z

charts/jaeger-v2/values.yaml

@@ -0,0 +1,62 @@
+service:


this file has a generic name. What will happen when you need to support Cassandra, which requires different configuration?

Signed-off-by: mehul <mehulsharam4786@gmail.com>

yurishkuro · 2024-10-13T15:29:31Z

charts/jaeger-v2/templates/deployment.yaml

+          args: 
+            - "--config"
+            - "/etc/jaeger/config.yaml"
+          ports:


missing the main OTLP ports

what are the port number and name i should put

all ports are defined here: https://github.com/jaegertracing/jaeger/blob/main/cmd/jaeger/Dockerfile

charts/jaeger-v2/values.yaml

Signed-off-by: mehul <mehulsharam4786@gmail.com>

yurishkuro · 2024-10-15T01:12:07Z

charts/jaeger-v2/Chart.yaml

@@ -0,0 +1,45 @@
+apiVersion: v2
+appVersion: 1.53.0


what is the meaning of this version? Is this a reference to Jaeger version? Should it be hardcoded like that?

The appVersion is only informational according to Helm https://helm.sh/docs/topics/charts/#the-appversion-field but it is used to mark the version of image tag if tag is not set. You can see here (several places) for example query-deploy.yaml calls query.image which uses renderImage which defaults the image to use Chart.AppVersion. This ultimately is also used in the bitnami common chart that is referenced on the line below.

So YES it should be hardcoded (AND UPDATED!), but it can be removed IF replaced by tag in the image: section. But given Jaeger uses a single binary for all deploys, this is a good global place to ensure they are all up to the same version.

yurishkuro · 2024-10-15T01:13:01Z

charts/jaeger-v2/values.yaml

+  jaeger_query:
+    storage:
+      traces: primary_store
+      traces_archive: another_store


Suggested change

traces_archive: another_store

traces_archive: archive_store

yurishkuro · 2024-10-15T01:13:15Z

charts/jaeger-v2/values.yaml

+      primary_store:
+        memory:
+          max_traces: 100000
+      another_store:


Suggested change

another_store:

archive_store:

yurishkuro · 2024-10-15T01:13:28Z

charts/jaeger-v2/values.yaml

+  jaeger:
+    protocols:
+      grpc:
+


Suggested change

Stevenpc3 · 2024-10-15T14:39:46Z

charts/jaeger-v2/Chart.yaml

+description: A Jaeger Helm chart for Kubernetes
+name: jaeger-v2
+type: application
+version: 3.3.1


would this not be version 1.0.0 or 0.1.0 or 1.0.0-rc1 ? Certainly we wouldn't want to start a new chart at 3.3.1?

Upon some review, this has a lot of major regressions compared to the other highly customizable chart. Is this just a draft?

Does version 2 not require a service.yaml and is it not a statefulset?
Service accounts?
Do we not need customization for query, collecter (are they gone in v2 of jeager)?
Nothing for elasticsearch cleaning or indexes or customization?
Oauth?
Spark?

There just seems to be a lot missing here and this is not marked draft or anything

@Stevenpc3 right now this is a proof of concept. I 100% agree with you that there are a lot of moving parts that could be reused from v1 chart, but I don't see a practical path towards that unless existing maintainers in this repo take on that work. This PR is being done by an intern who has both limited knowledge and limited time to get something done to enable Jaeger v2 installs.

Ok. I get it... I know what it's like lol. I didn't mean to seem harsh. I just rely on the Jaeger chart and have some PRs coming and a few contributes already. I was here looking at future updates/plans/paths and got as bit worried. Charts can be fixed, but the churn can have cascading effects. I will help where I can and certainly review. I can get other eyes on it to if needed. I just was thinking it would be published in an incomplete state.

Stevenpc3 · 2024-10-15T14:42:02Z

charts/jaeger-v2/Chart.yaml

+apiVersion: v2
+appVersion: 1.53.0
+description: A Jaeger Helm chart for Kubernetes
+name: jaeger-v2


I am a bit concerned about starting a jaeger-v2 chart.

Is the plan to deprecate the "jaeger" chart? Would all the template files in the jaeger chart be referenced here or copied over and duplicated?

why not make the templates backwards compatible and just keep the jaeger chart? If you cannot and it is cleaner to start a new one then I would recommend an overhaul and make a common folder to split up the jaeger and jeager-v2 charts then deprecate and eventually remove the jaeger chart and templates that no longer apply.

Stevenpc3 · 2024-10-15T14:50:21Z

charts/jaeger-v2/readme.md

+
+This Helm chart comes with pre-packaged dependencies for data stores:
+- **Cassandra**: Version `0.15.3`
+- **Elasticsearch**: Version `20.0.4`


you should probably remove the versions here. Unless using a template like 'helm-doc' to template and generate these versions, they will get out of date and not aligned.

Stevenpc3 · 2024-10-15T15:06:24Z

charts/jaeger-v2/templates/config-map.yaml

+kind: ConfigMap
+metadata:
+  name: {{ include "jaeger-v2.fullname" . }}
+  namespace: {{ template "jaeger-v2.namespace" . }}


why are you using template instead of include? https://stackoverflow.com/questions/71086697/how-does-template-and-include-differ-in-helm

Stevenpc3 · 2024-10-15T15:09:16Z

charts/jaeger-v2/values.yaml

+  jaeger_storage_exporter:
+    trace_storage: primary_store
+
+image: "jaegertracing/jaeger:2.0.0-rc2"


this should be broken up into registry, repo, image, tag to allow better overriding.

Stevenpc3 · 2024-10-15T15:10:33Z

charts/jaeger-v2/templates/deployment.yaml

+            - containerPort: 9411
+              name: zipkin
+
+          resources:


resources need to be overridable

Signed-off-by: mehul <mehulsharam4786@gmail.com>

yurishkuro · 2024-10-15T21:47:01Z

charts/jaeger-v2/Chart.yaml

+    email: jkowall@kowall.net
+dependencies:
+  - name: cassandra
+    version: 12.0.3


do you need to regenerate the lock file above?

Signed-off-by: mehul <mehulsharam4786@gmail.com>

hellspawn679 added 2 commits October 1, 2024 15:49

added all-in-one deployment and configmap for jaeger-v2

8fed143

Signed-off-by: Mehul <mehulsharma4786@gmail.com>

lint fix

783ab93

Signed-off-by: Mehul <mehulsharma4786@gmail.com>

yurishkuro reviewed Oct 1, 2024

View reviewed changes

hellspawn679 added 5 commits October 1, 2024 17:32

fixed

6dba3c9

Signed-off-by: Mehul <mehulsharma4786@gmail.com>

lint fix

f858e8a

Signed-off-by: Mehul <mehulsharma4786@gmail.com>

lint fix

9439e21

Signed-off-by: Mehul <mehulsharma4786@gmail.com>

fixed using pre-hook

82e4839

Signed-off-by: Mehul <mehulsharma4786@gmail.com>

Merge pull request #3 from hellspawn679/jaeger-v2-test

9cf70cb

Jaeger v2 test

hellspawn679 requested a review from yurishkuro October 1, 2024 23:51

mehul and others added 8 commits October 7, 2024 20:26

fixed --config flag is not been passed

e45d3b5

Signed-off-by: mehul <mehulsharam4786@gmail.com>

release ns for config-map.yaml

84e7a07

Signed-off-by: mehul <mehulsharam4786@gmail.com>

testing ci

38d3708

Signed-off-by: mehul <mehulsharam4786@gmail.com>

testing ci

6bc9dcc

Signed-off-by: mehul <mehulsharam4786@gmail.com>

fixed ns

be93c14

Signed-off-by: mehul <mehulsharam4786@gmail.com>

fixed ns

aac8af9

Signed-off-by: mehul <mehulsharam4786@gmail.com>

Merge pull request #5 from hellspawn679/jaeger-v2-test2

ae173e5

Jaeger v2 test2

fixed template

2bccbc8

Signed-off-by: mehul <mehulsharam4786@gmail.com>

yurishkuro reviewed Oct 11, 2024

View reviewed changes

charts/jaeger-v2/Chart.lock Outdated Show resolved Hide resolved

yurishkuro reviewed Oct 11, 2024

View reviewed changes

charts/jaeger-v2/Chart.lock Show resolved Hide resolved

yurishkuro reviewed Oct 11, 2024

View reviewed changes

mehul and others added 4 commits October 11, 2024 22:42

removed sampling

0cec95b

Signed-off-by: mehul <mehulsharam4786@gmail.com>

removed adaptive sampling from processors

144e55f

Signed-off-by: mehul <mehulsharam4786@gmail.com>

Merge pull request #7 from hellspawn679/jaeger-v2-test2

9aafe0c

Jaeger v2 test2

fixed lint

12e508f

Signed-off-by: mehul <mehulsharam4786@gmail.com>

yurishkuro reviewed Oct 11, 2024

View reviewed changes

charts/jaeger-v2/templates/config-map.yaml Show resolved Hide resolved

yurishkuro changed the title ~~added all-in-one deployment and configmap for jaeger-v2~~ Add all-in-one deployment and configmap for jaeger-v2 Oct 11, 2024

yurishkuro reviewed Oct 11, 2024

View reviewed changes

mehul added 4 commits October 12, 2024 02:29

fixed some template

6985100

Signed-off-by: mehul <mehulsharam4786@gmail.com>

fixed syntax problem in charts/jaeger-v2/config-map

0fae1f7

Signed-off-by: mehul <mehulsharam4786@gmail.com>

added labels

c0fd530

Signed-off-by: mehul <mehulsharam4786@gmail.com>

fixed lint

c4ac26b

Signed-off-by: mehul <mehulsharam4786@gmail.com>

hellspawn679 requested a review from yurishkuro October 12, 2024 17:40

yurishkuro reviewed Oct 13, 2024

View reviewed changes

charts/jaeger-v2/values.yaml Outdated Show resolved Hide resolved

yurishkuro reviewed Oct 13, 2024

View reviewed changes

charts/jaeger-v2/values.yaml Outdated Show resolved Hide resolved

fixed

54a3248

Signed-off-by: mehul <mehulsharam4786@gmail.com>

hellspawn679 requested a review from yurishkuro October 14, 2024 08:59

yurishkuro reviewed Oct 15, 2024

View reviewed changes

charts/jaeger-v2/values.yaml

jaeger:

protocols:

grpc:

Copy link

Member

yurishkuro Oct 15, 2024

Choose a reason for hiding this comment

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

Suggested change

Stevenpc3 reviewed Oct 15, 2024

View reviewed changes

charts/jaeger-v2/templates/deployment.yaml

- containerPort: 9411

name: zipkin

resources:

Copy link

Contributor

Stevenpc3 Oct 15, 2024

Choose a reason for hiding this comment

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

resources need to be overridable

fixed

03973fb

Signed-off-by: mehul <mehulsharam4786@gmail.com>

yurishkuro reviewed Oct 15, 2024

View reviewed changes

mehul added 2 commits October 16, 2024 03:23

regenerated chart.lock

e2425eb

Signed-off-by: mehul <mehulsharam4786@gmail.com>

fix lint

2960a50

Signed-off-by: mehul <mehulsharam4786@gmail.com>

hellspawn679 closed this Oct 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add all-in-one deployment and configmap for jaeger-v2 #606

Add all-in-one deployment and configmap for jaeger-v2 #606

hellspawn679 commented Oct 1, 2024 •

edited

Loading

yurishkuro Oct 1, 2024

hellspawn679 Oct 11, 2024

yurishkuro Oct 11, 2024

yurishkuro Oct 11, 2024

hellspawn679 Oct 13, 2024

yurishkuro Oct 11, 2024

hellspawn679 Oct 13, 2024

yurishkuro Oct 11, 2024

hellspawn679 Oct 11, 2024

yurishkuro Oct 12, 2024

yurishkuro Oct 11, 2024

hellspawn679 Oct 11, 2024

yurishkuro Oct 12, 2024

yurishkuro Oct 11, 2024

hellspawn679 Oct 13, 2024

yurishkuro Oct 11, 2024

yurishkuro Oct 13, 2024

hellspawn679 Oct 13, 2024

yurishkuro Oct 15, 2024

yurishkuro Oct 15, 2024

Stevenpc3 Oct 15, 2024

yurishkuro Oct 15, 2024

yurishkuro Oct 15, 2024

yurishkuro Oct 15, 2024

Stevenpc3 Oct 15, 2024

Stevenpc3 Oct 15, 2024

yurishkuro Oct 15, 2024

Stevenpc3 Oct 15, 2024 •

edited

Loading

Stevenpc3 Oct 15, 2024

Stevenpc3 Oct 15, 2024

Stevenpc3 Oct 15, 2024

Stevenpc3 Oct 15, 2024

Stevenpc3 Oct 15, 2024

yurishkuro Oct 15, 2024

		trace_storage: some_store


		image: "jaegertracing/jaeger:latest"

Add all-in-one deployment and configmap for jaeger-v2 #606

Add all-in-one deployment and configmap for jaeger-v2 #606

Conversation

hellspawn679 commented Oct 1, 2024 • edited Loading

What this PR does

Which issue this PR fixes

Checklist

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stevenpc3 Oct 15, 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

hellspawn679 commented Oct 1, 2024 •

edited

Loading

Stevenpc3 Oct 15, 2024 •

edited

Loading