[jaeger-v2] Add support for Elasticsearch #5152

akagami-harsh · 2024-01-29T16:19:21Z

Which problem is this PR solving?

part of [tracking] Jaeger v2 based on OpenTelemetry collector #4843

Description of the changes

add support for elasticsearch in jaeger-v2

How was this change tested?

tested locally

Checklist

I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
I have signed all commits
I have added unit tests for the new functionality
I have run lint and test steps successfully
- for jaeger: make lint test
- for jaeger-ui: yarn lint and yarn test

Signed-off-by: Harshvir Potpose <[email protected]>

codecov · 2024-01-29T20:31:06Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.49%. Comparing base (8a5f824) to head (278d49a).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5152      +/-   ##
==========================================
+ Coverage   94.48%   94.49%   +0.01%     
==========================================
  Files         334      334              
  Lines       19267    19303      +36     
==========================================
+ Hits        18204    18241      +37     
  Misses        875      875              
+ Partials      188      187       -1

Flag	Coverage Δ
cassandra-3.x	`25.53% <ø> (ø)`
cassandra-4.x	`25.53% <ø> (ø)`
elasticsearch-5.x	`19.83% <ø> (ø)`
elasticsearch-6.x	`19.81% <ø> (ø)`
elasticsearch-7.x	`19.94% <ø> (-0.02%)`	⬇️
elasticsearch-8.x	`20.03% <ø> (-0.02%)`	⬇️
grpc-badger	`19.46% <ø> (ø)`
kafka	`14.06% <ø> (ø)`
opensearch-1.x	`19.96% <ø> (ø)`
opensearch-2.x	`19.94% <ø> (-0.02%)`	⬇️
unittests	`92.35% <100.00%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cmd/jaeger/es_config.yaml

Signed-off-by: Harshvir Potpose <[email protected]>

cmd/jaeger/internal/extension/jaegerstorage/extension.go

yurishkuro · 2024-02-05T21:29:03Z

plugin/storage/es/factory.go

+	f := NewFactory()
+	f.InitFromOptions(Options{
+		Primary: namespaceConfig{Configuration: cfg},
+		others:  make(map[string]*namespaceConfig),


I wonder, if only Primary and not Archive is ever initialized, would it actually provide an archive storage to the UI? Did you try it out?

yes, you are right it dose not work with ui. it throw's this error

docker container used to test it

docker run --name es01 -p 9200:9200 -e "discovery.type=single-node" -e "xpack.security.enabled=false" elasticsearch:8.12.0

do you know what causes it?🤔 , i tried fixing it with different configuration of Options but couldn't fix.

Better to look in the server logs.

Can you put together a docker-compose that illustrates this issue?

This "all shards failed" error has always annoyed me, I wonder if there are more details in the error returned from ES that we're not logging that could be more descriptive. Like, it's ridiculous for a database to respond with "your query failed" instead of telling why (the fact that it failed on "all shards" is not an explanation of why).

Can you put together a docker-compose that illustrates this issue?

sure, i'll make one.

I didn't check this one previously because there were no errors in the UI. However, I've just ran it with elasticsearch and found this issue.

harsh@MSI:~$ curl localhost:16686 function getJaegerStorageCapabilities() { const DEFAULT_STORAGE_CAPABILITIES = { "archiveStorage": false }; const JAEGER_STORAGE_CAPABILITIES = {"archiveStorage":false}; return JAEGER_STORAGE_CAPABILITIES; }

when i checked with badger and memstore strangely {"archiveStorage":false} in both of them

At least for memstore you need to add archive to the config explicitly:

diff --git a/cmd/jaeger/internal/all-in-one.yaml b/cmd/jaeger/internal/all-in-one.yaml index d17c52fc..a6189c8e 100644 --- a/cmd/jaeger/internal/all-in-one.yaml +++ b/cmd/jaeger/internal/all-in-one.yaml @@ -9,11 +9,14 @@ service: extensions: jaeger_query: trace_storage: memstore + trace_storage_archive: memstore2 jaeger_storage: memory: memstore: max_traces: 100000 + memstore2: + max_traces: 100000

But maybe this is ok - the idea in v2 was to decouple primary and archive storage altogether, so that one could use different types of backends for those.

So in theory the config you have should've worked, because you did define archive storage, but something isn't working.

added a small fix for above problem, which sets {"archiveEnabled":true}

cmd/jaeger/internal/extension/jaegerstorage/extension.go

Co-authored-by: Yuri Shkuro <[email protected]> Signed-off-by: Harshvir Potpose <[email protected]>

Signed-off-by: Harshvir Potpose <[email protected]>

yurishkuro · 2024-02-06T20:58:56Z

please rebase on top of #5171

Signed-off-by: Harshvir Potpose <[email protected]>

cmd/jaeger/internal/extension/jaegerstorage/extension_test.go

yurishkuro · 2024-02-08T02:09:26Z

plugin/storage/es/factory.go

+	f := NewFactory()
+	f.InitFromOptions(Options{
+		Primary: namespaceConfig{Configuration: cfg},
+		others:  make(map[string]*namespaceConfig),


Better to look in the server logs.

yurishkuro · 2024-02-16T21:57:06Z

pkg/es/config/config.go

@@ -45,7 +45,7 @@ import (

 // Configuration describes the configuration properties needed to connect to an ElasticSearch cluster
 type Configuration struct {
-	Servers                        []string       `mapstructure:"server_urls"`
+	Servers                        []string       `mapstructure:"server_urls" validate:"required,min=1,dive,url"`


I wonder if there's a test we could write for ensuring that these annotations are correct (btw, what is "dive"?)

We could do it brute-force and just write individual tests for individual fields, but it seems redundant as that would be already done in the validator package (hopefully). But at the same time, having no tests to even ensure that the syntax of the annotations is correct is troublesome.

Comment for L58 and below - can you try removing yaml:... annotations? I think they are left-overs from earlier attempts to build on OTEL Coll, I don't think we're using them anywhere.

Comment for L58 and below - can you try removing yaml:... annotations? I think they are left-overs from earlier attempts to build on OTEL Coll, I don't think we're using them anywhere.

removed it

(btw, what is "dive"?)

dive instructs the validation library to examine each element within the slice individually

I wonder if there's a test we could write for ensuring that these annotations are correct

We could do it brute-force and just write individual tests for individual fields, but it seems redundant as that would be already done in the validator package (hopefully). But at the same time, having no tests to even ensure that the syntax of the annotations is correct is troublesome.

we can write unit tests that check whether the validation behaves as expected. Instead of testing individual fields, we can create a set of test cases that cover various scenarios, including both valid and invalid configurations.
should i create one?

but that wouldn't be what I am after, such test would simply verify that the Validator is working as expected, on select fields. But if I declare a field with annotations that don't make sense, I want to catch that in unit tests. E.g. misspell dive as diev (btw weird option, "recursive" would've been much more self explanatory).

We can use the "github.com/go-playground/validator" library in our unit tests to detect misspelled annotations and fields with invalid annotation combinations.

like this

package main import ( "github.com/go-playground/validator" ) type Webhook struct { CallbackURL string `validate:"url,badAnnotation"` // badAnnotation is not a valid tag } func main() { webhook := Webhook{ CallbackURL: "https://www.example.com/", } validate := validator.New() validate.Struct(webhook) }

>output panic: Undefined validation function 'badAnnotation' on field 'CallbackURL' goroutine 1 [running]: github.com/go-playground/validator.(*Validate).parseFieldTagsRecursive(0xc00002e240, {0x526987?, 0x55812d?}, {0x526971, 0xb}, {0x0, 0x0}, 0x0) /home/harsh/go/pkg/mod/github.com/go-playground/[email protected]+incompatible/cache.go:289 +0x9d9 github.com/go-playground/validator.(*Validate).extractStructCache(0xc00002e240, {0x538520?, 0xc00020a950?, 0x7fda1aac3198?}, {0x52160c, 0x7}) /home/harsh/go/pkg/mod/github.com/go-playground/[email protected]+incompatible/cache.go:150 +0x4ec github.com/go-playground/validator.(*validate).validateStruct(0xc000118480, {0x58b0c0, 0x68faa0}, {0x538520?, 0xc00020a950?, 0xc000242000?}, {0x538520?, 0xc00020a950?, 0x40e3a5?}, {0x58cd60, ...}, ...) /home/harsh/go/pkg/mod/github.com/go-playground/[email protected]+incompatible/validator.go:37 +0x196 github.com/go-playground/validator.(*Validate).StructCtx(0xc00002e240, {0x58b0c0, 0x68faa0}, {0x538520?, 0xc00020a950?}) /home/harsh/go/pkg/mod/github.com/go-playground/[email protected]+incompatible/validator_instance.go:304 +0x491 github.com/go-playground/validator.(*Validate).Struct(...) /home/harsh/go/pkg/mod/github.com/go-playground/[email protected]+incompatible/validator_instance.go:277 main.main() /home/harsh/Testing/main.go:16 +0x65 exit status 2

what about "github.com/asaskevich/govalidator", will it do the same? We do not use "github.com/go-playground/validator"

will it do the same?

Nope, it doesn't do the same. I actually tried using the existing validator at first.

Signed-off-by: Harshvir Potpose <[email protected]>

yurishkuro · 2024-02-18T20:59:07Z

cmd/jaeger/Dockerfile.debug

@@ -0,0 +1,11 @@
+FROM alpine:latest


not sure why you need this one - how is it different from the main Dockerfile?

I was having issues with the main Dockerfile so i created a simpler one as a solution. now we don't need the Dockerfile and Docker-compose file anymore, so I'm removing them.

cmd/jaeger/es_config.yaml

pkg/es/config/config.go

yurishkuro · 2024-02-18T21:12:58Z

plugin/storage/es/factory.go

+	f := NewFactory()
+	f.InitFromOptions(Options{
+		Primary: namespaceConfig{Configuration: cfg},
+		others:  make(map[string]*namespaceConfig),


so when I run v2 with your new config file (and without -tags ui), then placeholder UI shows this:

JAEGER_CONFIG = {"archiveEnabled":true}; JAEGER_STORAGE_CAPABILITIES = {"archiveStorage":false}; JAEGER_VERSION = {"gitCommit":"","gitVersion":"","buildDate":""};

We expect {"archiveStorage":true} in order for archiving to work

yurishkuro · 2024-02-18T22:11:24Z

I ran with your docker-compose and config and (with the help of #5209) it returns this error:

RootCause[Fielddata is disabled on [serviceName] in [jaeger-service-]. Text fields are not optimised for operations that require per-document field data like aggregations and sorting, so these operations are disabled by default. Please use a keyword field instead. Alternatively, set fielddata=true on [serviceName] in order to load field data by uninverting the inverted index. Note that this can use significant memory. [type=illegal_argument_exception]]

Signed-off-by: Harshvir Potpose <[email protected]>

akagami-harsh · 2024-02-20T12:10:34Z

I ran with your docker-compose and config and (with the help of #5209) it returns this error:

RootCause[Fielddata is disabled on [serviceName] in [jaeger-service-]. Text fields are not optimised for operations that require per-document field data like aggregations and sorting, so these operations are disabled by default. Please use a keyword field instead. Alternatively, set fielddata=true on [serviceName] in order to load field data by uninverting the inverted index. Note that this can use significant memory. [type=illegal_argument_exception]]

thanks a lot for the help. now i think it is working fine, setting UseReadWriteAliases to true fixes it.

Signed-off-by: Harshvir Potpose <[email protected]>

yurishkuro · 2024-02-25T23:00:46Z

plugin/storage/es/factory.go

+	archive[archiveNamespace] = &namespaceConfig{
+		Configuration: cfg,
+		namespace:     archiveNamespace,
+	}


ok, this is logically wrong, but will work for now. The underlying problem is the existence of storage.ArchiveFactory interface, we need to get rid of it long term, because as far as interface goes the archive storage is just like a regular storage, only with different configuration.

plugin/storage/es/factory.go

Signed-off-by: Harshvir Potpose <[email protected]>

yurishkuro

Thanks!

akagami-harsh and others added 4 commits January 29, 2024 21:40

add elasticsearch for jeager-v2

0afdfd5

Signed-off-by: Harshvir Potpose <[email protected]>

fix

c60bf5a

Signed-off-by: Harshvir Potpose <[email protected]>

Merge branch 'main' into elasticsearch

67707ed

add tests

a47bb35

Signed-off-by: Harshvir Potpose <[email protected]>

akagami-harsh marked this pull request as ready for review January 29, 2024 20:23

akagami-harsh requested a review from a team as a code owner January 29, 2024 20:23

akagami-harsh requested a review from jkowall January 29, 2024 20:23

akagami-harsh added 2 commits January 31, 2024 22:45

Merge branch 'main' into elasticsearch

5ea1df1

Merge branch 'main' into elasticsearch

70edeeb

yurishkuro reviewed Feb 5, 2024

View reviewed changes

cmd/jaeger/es_config.yaml Outdated Show resolved Hide resolved

use separate index_prefix in archive storage

430a0ce

Signed-off-by: Harshvir Potpose <[email protected]>

yurishkuro reviewed Feb 5, 2024

View reviewed changes

cmd/jaeger/internal/extension/jaegerstorage/extension.go Outdated Show resolved Hide resolved

yurishkuro reviewed Feb 5, 2024

View reviewed changes

cmd/jaeger/internal/extension/jaegerstorage/extension.go Outdated Show resolved Hide resolved

akagami-harsh and others added 3 commits February 6, 2024 12:51

Update cmd/jaeger/internal/extension/jaegerstorage/extension.go

d8d4939

Co-authored-by: Yuri Shkuro <[email protected]> Signed-off-by: Harshvir Potpose <[email protected]>

Update cmd/jaeger/internal/extension/jaegerstorage/extension.go

da38c6a

Co-authored-by: Yuri Shkuro <[email protected]> Signed-off-by: Harshvir Potpose <[email protected]>

fix

001e4a0

Signed-off-by: Harshvir Potpose <[email protected]>

yurishkuro added changelog:new-feature Change that should be called out as new feature in CHANGELOG v2 changelog:exprimental Change to an experimental part of the code and removed changelog:exprimental Change to an experimental part of the code labels Feb 6, 2024

yurishkuro added changelog:exprimental Change to an experimental part of the code and removed changelog:new-feature Change that should be called out as new feature in CHANGELOG labels Feb 7, 2024

akagami-harsh and others added 4 commits February 7, 2024 12:18

solve merge conflicts

f93b1c6

Signed-off-by: Harshvir Potpose <[email protected]>

refactor tests

9aa2c26

Signed-off-by: Harshvir Potpose <[email protected]>

fix

c5c20ce

Signed-off-by: Harshvir Potpose <[email protected]>

Merge branch 'main' into elasticsearch

bcdf8a0

yurishkuro reviewed Feb 8, 2024

View reviewed changes

yurishkuro reviewed Feb 16, 2024

View reviewed changes

akagami-harsh added 3 commits February 18, 2024 14:31

remove left over yaml annotations

79130b4

Signed-off-by: Harshvir Potpose <[email protected]>

add docker compose for debug

e373c3d

Signed-off-by: Harshvir Potpose <[email protected]>

fix

e646a3f

Signed-off-by: Harshvir Potpose <[email protected]>

yurishkuro reviewed Feb 18, 2024

View reviewed changes

akagami-harsh and others added 7 commits February 19, 2024 13:47

revert changes

e61cf78

Signed-off-by: Harshvir Potpose <[email protected]>

fix

6824ec4

Signed-off-by: Harshvir Potpose <[email protected]>

Merge branch 'jaegertracing:main' into elasticsearch

999d336

rename es_config.yaml to config-elasticsearch.yaml

66f63b5

Signed-off-by: Harshvir Potpose <[email protected]>

set UseReadWriteAliases to true in config file

a86760c

Signed-off-by: Harshvir Potpose <[email protected]>

remove debug docker compose and dockerfile

7ad3b0d

Signed-off-by: Harshvir Potpose <[email protected]>

add validator to validate config

8dc35cb

Signed-off-by: Harshvir Potpose <[email protected]>

akagami-harsh added 2 commits February 21, 2024 00:06

fix

0744562

Signed-off-by: Harshvir Potpose <[email protected]>

fix

275786c

Signed-off-by: Harshvir Potpose <[email protected]>

akagami-harsh requested a review from yurishkuro February 23, 2024 14:34

akagami-harsh added 2 commits February 26, 2024 01:55

fix

3763aa9

Signed-off-by: Harshvir Potpose <[email protected]>

fix lint

495daaf

Signed-off-by: Harshvir Potpose <[email protected]>

yurishkuro reviewed Feb 25, 2024

View reviewed changes

plugin/storage/es/factory.go Show resolved Hide resolved

akagami-harsh added 3 commits February 26, 2024 23:38

add test for cfg validation

0f7d2f1

Signed-off-by: Harshvir Potpose <[email protected]>

fix lint

a6cec26

Signed-off-by: Harshvir Potpose <[email protected]>

fix

915e83f

Signed-off-by: Harshvir Potpose <[email protected]>

yurishkuro approved these changes Feb 27, 2024

View reviewed changes

Merge branch 'main' into elasticsearch

278d49a

yurishkuro enabled auto-merge (squash) February 27, 2024 00:40

yurishkuro merged commit 7a04461 into jaegertracing:main Feb 27, 2024
35 checks passed

akagami-harsh deleted the elasticsearch branch March 17, 2024 07:49

yurishkuro mentioned this pull request Nov 9, 2024

[jaeger-v2] Standard way to validate configuration #6185

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[jaeger-v2] Add support for Elasticsearch #5152

[jaeger-v2] Add support for Elasticsearch #5152

akagami-harsh commented Jan 29, 2024 •

edited

Loading

codecov bot commented Jan 29, 2024 •

edited

Loading

yurishkuro Feb 5, 2024

akagami-harsh Feb 6, 2024 •

edited

Loading

yurishkuro Feb 8, 2024

yurishkuro Feb 16, 2024

akagami-harsh Feb 18, 2024 •

edited

Loading

akagami-harsh Feb 25, 2024 •

edited

Loading

akagami-harsh Feb 25, 2024

yurishkuro Feb 25, 2024

yurishkuro Feb 25, 2024

akagami-harsh Feb 25, 2024

yurishkuro commented Feb 6, 2024

yurishkuro Feb 8, 2024

yurishkuro Feb 16, 2024

yurishkuro Feb 16, 2024

akagami-harsh Feb 18, 2024

akagami-harsh Feb 18, 2024

akagami-harsh Feb 18, 2024

yurishkuro Feb 18, 2024

akagami-harsh Feb 20, 2024 •

edited

Loading

yurishkuro Feb 20, 2024

akagami-harsh Feb 20, 2024

yurishkuro Feb 18, 2024

akagami-harsh Feb 20, 2024 •

edited

Loading

yurishkuro Feb 18, 2024

yurishkuro commented Feb 18, 2024

akagami-harsh commented Feb 20, 2024

yurishkuro Feb 25, 2024

yurishkuro Feb 25, 2024

yurishkuro left a comment

[jaeger-v2] Add support for Elasticsearch #5152

[jaeger-v2] Add support for Elasticsearch #5152

Conversation

akagami-harsh commented Jan 29, 2024 • edited Loading

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

codecov bot commented Jan 29, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

akagami-harsh Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akagami-harsh Feb 18, 2024 • edited Loading

Choose a reason for hiding this comment

akagami-harsh Feb 25, 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

yurishkuro commented Feb 6, 2024

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

akagami-harsh Feb 20, 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

akagami-harsh Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro commented Feb 18, 2024

akagami-harsh commented Feb 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

akagami-harsh commented Jan 29, 2024 •

edited

Loading

codecov bot commented Jan 29, 2024 •

edited

Loading

akagami-harsh Feb 6, 2024 •

edited

Loading

akagami-harsh Feb 18, 2024 •

edited

Loading

akagami-harsh Feb 25, 2024 •

edited

Loading

akagami-harsh Feb 20, 2024 •

edited

Loading

akagami-harsh Feb 20, 2024 •

edited

Loading