Create sampling templates when creating sampling store #5349

JaeguKim · 2024-04-11T15:12:21Z

Which problem is this PR solving?

Resolves es.SamplingStore.CreateTemplates is never called #5333

Description of the changes

Create sampling templates when creating sampling store since doing so is intended.

How was this change tested?

CI

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

yurishkuro

Thanks!

The full scope of this ticket includes:

adding tests to validate this change
changing existing integration test that manually creates the mapping to not do that and instead rely on this new built-in behavior

Question: what happens if the mapping already exists?

plugin/storage/es/factory.go

codecov · 2024-04-11T15:52:38Z

Codecov Report

Attention: Patch coverage is 93.33333% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 95.19%. Comparing base (466e105) to head (b48c349).

Files	Patch %	Lines
plugin/storage/es/factory.go	91.66%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5349      +/-   ##
==========================================
- Coverage   95.21%   95.19%   -0.03%     
==========================================
  Files         343      343              
  Lines       16782    16785       +3     
==========================================
- Hits        15979    15978       -1     
- Misses        605      608       +3     
- Partials      198      199       +1

Flag	Coverage Δ
badger	`10.47% <0.00%> (-0.04%)`	⬇️
cassandra-3.x	`18.38% <0.00%> (-0.05%)`	⬇️
cassandra-4.x	`18.38% <0.00%> (-0.05%)`	⬇️
elasticsearch-5.x	`20.89% <73.33%> (+0.01%)`	⬆️
elasticsearch-6.x	`20.87% <73.33%> (-0.01%)`	⬇️
elasticsearch-7.x	`20.98% <80.00%> (+0.05%)`	⬆️
elasticsearch-8.x	`21.11% <73.33%> (+<0.01%)`	⬆️
grpc	`14.55% <6.66%> (-0.04%)`	⬇️
kafka	`10.17% <0.00%> (-0.01%)`	⬇️
opensearch-1.x	`20.98% <73.33%> (+<0.01%)`	⬆️
opensearch-2.x	`20.99% <73.33%> (+0.01%)`	⬆️
unittests	`91.71% <80.00%> (-0.02%)`	⬇️

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.

plugin/storage/es/factory.go

JaeguKim · 2024-04-12T13:57:18Z

@yurishkuro Apologies for the delayed response.
I've implemented tests to ensure coverage of code changes:

I've refactored the integration test to utilize the CreateSamplingStore method instead of manual creation.

0356039

When mapping was already created and SamplingStore is created again via CreateSamplingStore, this could result the existing indexes or entire indexes becoming unsearchable depending on the compatibility between existing and new index template.

I think there are several strategies.

Merge existing index template with the new template
- This approach involves handling cases, such as conflicting field types, to ensure that existing indexes remain searchable.
Create a new index template with a different name
- By supporting search for indexes with different index template versions, we can maintain compatibility while introducing the new template. But storage cost should be considered.

yurishkuro · 2024-04-12T14:19:19Z

for some reason this test consistently fails in your branch, while always succeeds in main:

$ go test ./plugin/storage/es
ok  	github.com/jaegertracing/jaeger/plugin/storage/es	(cached)

plugin/storage/es/factory.go

yurishkuro · 2024-04-12T14:22:35Z

When mapping was already created and SamplingStore is created again via CreateSamplingStore, this could result the existing indexes or entire indexes becoming unsearchable depending on the compatibility between existing and new index template.

why is that? doesn't CreateSamplingStore create different index templates? It should not be affecting span templates.

plugin/storage/es/factory.go

JaeguKim · 2024-04-12T14:48:11Z

why is that? doesn't CreateSamplingStore create different index templates? It should not be affecting span templates.

jaeger/plugin/storage/es/samplingstore/storage.go

Line 165 in aae3616

    
           _, err := s.client().CreateTemplate("jaeger-sampling").Body(samplingTemplate).Do(context.Background())

It appears that the template ID is hardcoded as jaeger-sampling. Am I interpreting this correctly?

plugin/storage/es/factory.go

plugin/storage/integration/elasticsearch_test.go

yurishkuro · 2024-04-14T17:05:09Z

seeing this failure

    --- ❌ FAIL: TestIndexCleaner/RemoveDailyIndices_with_adaptiveSampling_prefix,_[] (2.78s)
=== RUN   TestIndexRollover_FailIfILMNotPresent
Error: ILM policy jaeger-ilm-policy doesn't exist in Elasticsearch. Please create it and re-run init

plugin/storage/integration/elasticsearch_test.go

yurishkuro · 2024-04-15T16:11:29Z

@Pushkarm029 this is the PR I was referring to.

yurishkuro · 2024-04-15T16:13:03Z

@JaeguKim could you please rebase?

JaeguKim · 2024-04-17T16:24:29Z

@JaeguKim could you please rebase?

@yurishkuro I rebased!

JaeguKim · 2024-04-19T14:40:17Z

plugin/storage/integration/integration.go

@@ -299,6 +299,7 @@ func (s *StorageIntegration) testFindTraces(t *testing.T) {
 		}
 		expectedTracesPerTestCase = append(expectedTracesPerTestCase, expected)
 	}
+	time.Sleep(time.Second)


@yurishkuro I attempted to address the test failure occurring with ES7 by incorporating t.Parallel() to ensure the test keeps retrying until 100 seconds have elapsed. Despite this effort, the test continued to fail. While I understand that relying on time.Sleep is not ideal, I don't know other alternative solutions for now.

Sleep alone is not a reliable mechanism to deal with concurrency, retries are. We already have sleep inside waitForCondition, which is used by findTracesByQuery called in this test. Why doesn't that work?

It turns out that the retry condition in waitForCondition was incorrect. The retry condition should match not only the trace count but also the span count. I addressed this issue in commit 8c8f2e3. I'm curious why the test only fails intermittently when using other ES versions.

Signed-off-by: JaegooKim <rlaworn1993@gmail.com>

Signed-off-by: JaeguKim <rlaworn1993@gmail.com>

inline CreateTemplates function,improve naming Signed-off-by: JaeguKim <rlaworn1993@gmail.com>

Signed-off-by: JaeguKim <rlaworn1993@gmail.com>

Wrap error with message, improve naming Signed-off-by: JaeguKim <rlaworn1993@gmail.com>

Pass flags to factory Signed-off-by: JaeguKim <rlaworn1993@gmail.com>

Signed-off-by: JaeguKim <rlaworn1993@gmail.com>

Refactor code Signed-off-by: JaeguKim <rlaworn1993@gmail.com>

Signed-off-by: JaeguKim <rlaworn1993@gmail.com>

…n integration test Signed-off-by: JaeguKim <rlaworn1993@gmail.com>

Signed-off-by: JaeguKim <rlaworn1993@gmail.com>

…ing es7. Signed-off-by: JaeguKim <rlaworn1993@gmail.com>

Signed-off-by: JaeguKim <rlaworn1993@gmail.com>

yurishkuro · 2024-04-20T03:12:50Z

Thanks!

Pushkarm029 · 2024-04-23T18:49:48Z

I think we still get this error. Because CreateLock() for es is yet to be implemented.

yurishkuro · 2024-04-23T20:45:12Z

Because CreateLock() for es is yet to be implemented.

yeah, we should add this to integration test

…#5349) ## Which problem is this PR solving? - Resolves jaegertracing#5333 ## Description of the changes - Create sampling templates when creating sampling store since doing so is intended. ## How was this change tested? - CI ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [ ] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` --------- Signed-off-by: JaeguKim <rlaworn1993@gmail.com> Signed-off-by: Vamshi Maskuri <gwcchintu@gmail.com>

JaeguKim requested a review from a team as a code owner April 11, 2024 15:12

JaeguKim requested a review from pavolloffay April 11, 2024 15:12

JaeguKim force-pushed the create-templates-when-creating-sampling-store branch from 1f2d4c6 to 4e8c627 Compare April 11, 2024 15:14

yurishkuro reviewed Apr 11, 2024

View reviewed changes