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] Add support for Elasticsearch #617

Merged
merged 33 commits into from
Nov 19, 2024

Conversation

hellspawn679
Copy link
Contributor

@hellspawn679 hellspawn679 commented Nov 8, 2024

What this PR does

added support for elasticsearch in jaeger-v2 helm chart

Which issue this PR fixes

(optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format,
will close that issue when PR gets merged)

  • fixes add support for elasticsearch for jaeger v2

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

rgaduput and others added 21 commits October 21, 2024 02:04
Signed-off-by: Reddysekhar Gaduputi <gsekhar73@gmail.com>

upgrade jaeger-operator to latest 1.61.0 (jaegertracing#605)

Signed-off-by: Blair Bowden <blair@radiusmethod.com>

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

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

lint fix

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

fixed

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

lint fix

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

lint fix

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

fixed using pre-hook

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

fixed --config flag is not been passed

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

release ns for config-map.yaml

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

testing ci

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

testing ci

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

fixed ns

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

fixed ns

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

fixed template

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

removed sampling

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

removed adaptive sampling from processors

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

Revert "Jaeger v2 test2"

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

attempt to create v2 chart in v1

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

enabled collector query and agent

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

version bump

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

testing-v2-ci

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

testing-v2-ci

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

added --helm-extra-set-args flag

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

fixed healthcheck port-v2

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

Fix health check path

Signed-off-by: Yuri Shkuro <github@ysh.us>

minor changes

Signed-off-by: mehul <mehulsharam4786@gmail.com>
Signed-off-by: mehul gautam  <mehulsharma4786@gmail.com>
Signed-off-by: mehul <mehulsharam4786@gmail.com>
Signed-off-by: mehul <mehulsharam4786@gmail.com>
Signed-off-by: mehul <mehulsharam4786@gmail.com>
Signed-off-by: mehul <mehulsharam4786@gmail.com>
Signed-off-by: mehul <mehulsharam4786@gmail.com>
Signed-off-by: mehul <mehulsharam4786@gmail.com>
Signed-off-by: mehul <mehulsharam4786@gmail.com>
Signed-off-by: mehul <mehulsharam4786@gmail.com>
Signed-off-by: mehul <mehulsharam4786@gmail.com>
Signed-off-by: mehul <mehulsharam4786@gmail.com>
Signed-off-by: mehul <mehulsharam4786@gmail.com>
Signed-off-by: mehul <mehulsharam4786@gmail.com>
Signed-off-by: mehul gautam  <mehulsharma4786@gmail.com>
Signed-off-by: mehul <mehulsharma4786@gmail.com>
Signed-off-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
Signed-off-by: mehul <mehulsharma4786@gmail.com>
Signed-off-by: mehul <mehulsharma4786@gmail.com>
Signed-off-by: mehul <mehulsharma4786@gmail.com>
Signed-off-by: mehul gautam  <mehulsharma4786@gmail.com>
@yurishkuro
Copy link
Member

please write complete PR title and description

backends:
some_storage:
elasticsearch:
index_prefix: {{- .Values.storage.elasticsearch.index_prefix | quote | indent 2 }}
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this. Values.storage.elasticsearch.index_prefix is a separate domain-specific configuration language that exists only in the Operator - why do we need it? The users can provide their own custom config if they want to customize these things. If we still want to allow overriding specific fields of the built-in configuration, then those overrides should be using the OTEL config syntax, e.g. Values.config.extensions.jaeger_storage...etc...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to keep the flag syntax same as jaeger-v1 so if the users wants to run it with old flag he can set it that way and helm will use es-user-conifg.yaml for that but if the user wants to provide there own config he can use the -set-file flag in that case we will use user-config.yaml

Copy link
Member

Choose a reason for hiding this comment

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

The old chart had no choice but to invent a new configuration scheme since jaeger-v1 did not support configuration, only cli flags. V2 supports configuration, so using a different scheme in the operator means the user needs to learn two different schemes, jaeger config and operator config. We need to avoid it. Use new scheme only when absolutely necessary, like in provision storage instructions, but for everything that can already be expressed in otel config just use that config directly.

user-config.yaml Outdated
@@ -0,0 +1,45 @@
service:
Copy link
Member

Choose a reason for hiding this comment

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

should this file be called user-config.yaml? It's ES-specific.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, if the user provides their own config, this will be used instead of es-user-config.yaml.

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 file meant to be an example config?

Copy link
Member

Choose a reason for hiding this comment

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

Please add comments in the header explaining the purpose of the file

Copy link
Member

Choose a reason for hiding this comment

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

?

Signed-off-by: mehul <mehulsharma4786@gmail.com>
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

please document in the README how the user is expected to use the chart with ES, using different options such as providing params via --set or a full config via --set-file

charts/jaeger/templates/es-configmap.yaml Outdated Show resolved Hide resolved
charts/jaeger/values.yaml Outdated Show resolved Hide resolved
user-config.yaml Outdated
@@ -0,0 +1,45 @@
service:
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 file meant to be an example config?

Signed-off-by: mehul <mehulsharma4786@gmail.com>
@yurishkuro yurishkuro changed the title [Jaeger] Jaeger v2 with v1 [jaeger-v2] Add support for Elasticsearch Nov 13, 2024
.github/workflows/lint-test.yaml Outdated Show resolved Hide resolved
.github/workflows/lint-test.yaml Outdated Show resolved Hide resolved
charts/jaeger/README-v2.md Outdated Show resolved Hide resolved
.github/workflows/lint-test.yaml Outdated Show resolved Hide resolved
charts/jaeger/templates/es-configmap.yaml Show resolved Hide resolved
charts/jaeger/templates/es-configmap.yaml Outdated Show resolved Hide resolved
charts/jaeger/values.yaml Show resolved Hide resolved
charts/jaeger/values.yaml Outdated Show resolved Hide resolved
user-config.yaml Outdated
@@ -0,0 +1,45 @@
service:
Copy link
Member

Choose a reason for hiding this comment

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

Please add comments in the header explaining the purpose of the file

Signed-off-by: mehul <mehulsharma4786@gmail.com>
Signed-off-by: mehul <mehulsharma4786@gmail.com>
@@ -4,6 +4,8 @@ description: A Jaeger Helm chart for Kubernetes
name: jaeger
type: application
version: 4.0.0
annotations:
Jaegerv1Version: "1.62.0"
Copy link
Member

Choose a reason for hiding this comment

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

Can you elaborate why this variable is defined in annotations instead of values.yaml? What is the significance of having it here?

Signed-off-by: mehul <mehulsharma4786@gmail.com>
Signed-off-by: mehul <mehulsharma4786@gmail.com>
Signed-off-by: mehul <mehulsharma4786@gmail.com>
Signed-off-by: mehul <mehulsharma4786@gmail.com>
Signed-off-by: mehul <mehulsharma4786@gmail.com>
Signed-off-by: mehul <mehulsharma4786@gmail.com>
@yurishkuro yurishkuro merged commit e9655bc into jaegertracing:v2 Nov 19, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants