2813 - Move all mapping related code to mappings package. #2822

bhiravabhatla · 2021-02-22T04:16:11Z

Signed-off-by: santosh [email protected]

Resolves #2813

Which problem is this PR solving?

Move all mapping related code to mappings package.
Implement Feedback on refactoring mappings code

Short description of the changes

Create a struct SpanServiceMappingsBuilder and move all functions like GetSpanServiceMappings, FixMapping, LoadMapping to take that struct as receiver instead of passing a dozen parameters to them.
Move all the mapping-related business into mappings package, instead of being in the factory
Rename ESPrefix to IndexPrefix to be more readable

bhiravabhatla · 2021-02-22T04:19:43Z

@albertteoh /@yurishkuro. Could not implement below points -

This will also allow to omit assigning the es.TemplateBuilder field (it should default to es.TextTemplateBuilder{} automatically), keeping the code cleaner yet still providing testability.
Use that struct directly to pass values to tmpl.Execute(), instead of declaring an inner struct.

As I needed to pass the template builder interface for mocking in tests.

bhiravabhatla · 2021-02-22T04:20:20Z

Would look at failing integration tests.

codecov · 2021-02-22T04:27:46Z

Codecov Report

Merging #2822 (3922dcc) into master (c01aa2b) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2822      +/-   ##
==========================================
+ Coverage   95.92%   95.95%   +0.03%     
==========================================
  Files         222      223       +1     
  Lines        9696     9695       -1     
==========================================
+ Hits         9301     9303       +2     
+ Misses        325      323       -2     
+ Partials       70       69       -1

Impacted Files	Coverage Δ
cmd/esmapping-generator/app/flags.go	`100.00% <100.00%> (ø)`
cmd/esmapping-generator/app/renderer/render.go	`100.00% <100.00%> (ø)`
plugin/storage/es/dependencystore/storage.go	`91.11% <100.00%> (ø)`
plugin/storage/es/factory.go	`97.61% <100.00%> (-0.60%)`	⬇️
plugin/storage/es/mappings/mapping.go	`100.00% <100.00%> (ø)`
plugin/storage/integration/integration.go	`77.90% <0.00%> (+0.55%)`	⬆️
cmd/query/app/server.go	`97.08% <0.00%> (+1.45%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c01aa2b...3922dcc. Read the comment docs.

plugin/storage/es/mappings/mapping.go

bhiravabhatla · 2021-02-22T04:51:55Z

Wierd, integration tests passed locally on elasticsearch v7.3.0.

bhiravabhatla · 2021-02-22T05:51:20Z

Wierd, integration tests passed locally on elasticsearch v7.3.0.

Ok, I am able reproduce locally as well. Not entirely sure whats causing this though. Will spend some time on this

bhiravabhatla · 2021-02-22T06:03:39Z

I am seeing this in my elasticsearch logs during the test runs.

"stacktrace": ["org.elasticsearch.transport.RemoteTransportException: [ca859ca2c689][127.0.0.1:9300][indices:data/read/search[phase/query]]", "Caused by: java.lang.IllegalArgumentException: Fielddata is disabled on text fields by default. Set fielddata=true on [serviceName] in order to load fielddata in memory by uninverting the inverted index. Note that this can however use significant memory. Alternatively use a keyword field instead."
@albertteoh / @yurishkuro - Any idea on this.

albertteoh · 2021-02-22T11:02:02Z

Fielddata is disabled on text fields by default. Set fielddata=true on [serviceName] ...

I saw this once when I had index mapping issues; I think it means your serviceName field was set to type text which is what ES sets for strings if there is no existing template (dynamic mapping). If it's type text, it means individual words in the string cannot be indexed, I think.

I would suggest querying ES on both your span and service mappings to confirm this is indeed the case (type is text instead of keyword):

GET jaeger-span-write/_mapping
GET jaeger-service-write/_mapping

And check your templates to ensure serviceName is present and correct (should set type to keyword) in both the mappings:

GET _template/jaeger-span
GET _template/jaeger-service

And from there, it's just a matter of pinning down the root cause from what's changed in this PR.

bhiravabhatla · 2021-02-24T13:50:23Z

Fielddata is disabled on text fields by default. Set fielddata=true on [serviceName] ...

I saw this once when I had index mapping issues; I think it means your serviceName field was set to type text which is what ES sets for strings if there is no existing template (dynamic mapping). If it's type text, it means individual words in the string cannot be indexed, I think.

I would suggest querying ES on both your span and service mappings to confirm this is indeed the case (type is text instead of keyword):
GET jaeger-span-write/_mapping
GET jaeger-service-write/_mapping
And check your templates to ensure serviceName is present and correct (should set type to keyword) in both the mappings:
GET _template/jaeger-span
GET _template/jaeger-service
And from there, it's just a matter of pinning down the root cause from what's changed in this PR.

Thank you @albertteoh - that helped. Index template pattern was the issue, index prefix was getting suffixed with "-" twice. Added a suffix check to avoid it. Tests passed locally.

plugin/storage/es/mappings/gen_assets.go

plugin/storage/es/mappings/jaeger-dependencies.json

yurishkuro · 2021-02-25T03:36:29Z

plugin/storage/es/mappings/mapping_test.go

+		args                       args
+		mockNewTextTemplateBuilder func() es.TemplateBuilder


I would just use MappingBuilder struct directly, there's no benefit in the indirections

Dint get this.

rather than defining a struct for tests and copying values into it, just use MappingBuilder struct directly when defining test cases.

Not part of this commit. Will work on it now.

albertteoh · 2021-02-25T05:25:01Z

plugin/storage/es/mappings/mapping_test.go

+			want := ""
+			if tt.esVersion == 7 {
+				want, err = mb.fixMapping("/" + tt.mapping + "-7.json")
+				require.NoError(t, err)


nit: could move this to after the else to remove duplication

@albertteoh dint get this - you want me to move require.NoError(t, err) after else?

plugin/storage/es/mappings/mapping_test.go

plugin/storage/es/mappings/mapping.go

plugin/storage/es/mappings/mapping_test.go

…yurishkuro

…yurishkuro's feedback on refactoring Signed-off-by: santosh <[email protected]>

Signed-off-by: santosh <[email protected]>

…kuro

…kuro's feedback Signed-off-by: santosh <[email protected]>

Signed-off-by: santosh <[email protected]>

…tes & implement feedback on tests Signed-off-by: santosh <[email protected]>

yurishkuro · 2021-02-26T22:49:08Z

plugin/storage/es/mappings/mapping.go

+
+//go:embed *.json
+// MAPPINGS contains embeded index templates.
+var MAPPINGS embed.FS


does this need to be public?

Suggested change

var MAPPINGS embed.FS

var mappingsFS embed.FS

@yurishkuro

…#2822) * 2813 - Move all mapping related code to mappings package & implement @yurishkuro's feedback on refactoring Signed-off-by: santosh <[email protected]> * 2813 - Fix name of the index templates Signed-off-by: santosh <[email protected]> * 2813 - Rename ESPrefix to IndexPrefix for clarity & Implement @yurishkuro's feedback Signed-off-by: santosh <[email protected]> * 2813 - Fix fmt issue Signed-off-by: santosh <[email protected]> * 2813 - Fix issue with adding hyphen to index prefix Signed-off-by: santosh <[email protected]> * 2813 - Used golang embed package instead of esc to embed index templates & implement feedback on tests Signed-off-by: santosh <[email protected]>

bhiravabhatla requested a review from a team as a code owner February 22, 2021 04:16

bhiravabhatla requested a review from albertteoh February 22, 2021 04:16

yurishkuro reviewed Feb 22, 2021

View reviewed changes

plugin/storage/es/mappings/mapping.go Outdated Show resolved Hide resolved

plugin/storage/es/mappings/mapping.go Outdated Show resolved Hide resolved

bhiravabhatla force-pushed the 2813 branch from 3d7cf45 to 5f0baeb Compare February 24, 2021 13:49

yurishkuro reviewed Feb 25, 2021

View reviewed changes

plugin/storage/es/mappings/gen_assets.go Outdated Show resolved Hide resolved

yurishkuro reviewed Feb 25, 2021

View reviewed changes

plugin/storage/es/mappings/jaeger-dependencies.json Show resolved Hide resolved

yurishkuro reviewed Feb 25, 2021

View reviewed changes

albertteoh reviewed Feb 25, 2021

View reviewed changes

yurishkuro mentioned this pull request Feb 26, 2021

Support embedding assets from a subdir #2846

Merged

bhiravabhatla added 6 commits February 26, 2021 11:15

2813 - Move all mapping related code to mappings package & implement @…

1ffd41e

…yurishkuro's feedback on refactoring Signed-off-by: santosh <[email protected]>

2813 - Fix name of the index templates

60620d6

Signed-off-by: santosh <[email protected]>

2813 - Rename ESPrefix to IndexPrefix for clarity & Implement @yurish…

83adcb7

…kuro's feedback Signed-off-by: santosh <[email protected]>

2813 - Fix fmt issue

dff8679

Signed-off-by: santosh <[email protected]>

2813 - Fix issue with adding hyphen to index prefix

3003c46

Signed-off-by: santosh <[email protected]>

2813 - Used golang embed package instead of esc to embed index templa…

3922dcc

…tes & implement feedback on tests Signed-off-by: santosh <[email protected]>

bhiravabhatla force-pushed the 2813 branch from 51a15a9 to 3922dcc Compare February 26, 2021 06:05

yurishkuro reviewed Feb 26, 2021

View reviewed changes

yurishkuro merged commit e4cacd0 into jaegertracing:master Feb 26, 2021

yurishkuro mentioned this pull request Feb 27, 2021

Replace the use of "esc" with standard "embed" package #2749

Closed

6 tasks

dependabot bot mentioned this pull request Mar 12, 2021

Bump jaeger from 72b9a06 to 78019b6 querycap/jaeger#35

Closed

jpkrohling added this to the Release 1.23.0 milestone Jun 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2813 - Move all mapping related code to mappings package. #2822

2813 - Move all mapping related code to mappings package. #2822

bhiravabhatla commented Feb 22, 2021 •

edited

Loading

bhiravabhatla commented Feb 22, 2021

bhiravabhatla commented Feb 22, 2021

codecov bot commented Feb 22, 2021 •

edited

Loading

bhiravabhatla commented Feb 22, 2021

bhiravabhatla commented Feb 22, 2021

bhiravabhatla commented Feb 22, 2021

albertteoh commented Feb 22, 2021 •

edited

Loading

bhiravabhatla commented Feb 24, 2021

yurishkuro Feb 25, 2021

bhiravabhatla Feb 26, 2021

yurishkuro Feb 26, 2021

bhiravabhatla Feb 26, 2021

albertteoh Feb 25, 2021

bhiravabhatla Feb 26, 2021

yurishkuro Feb 26, 2021

		args args
		mockNewTextTemplateBuilder func() es.TemplateBuilder

2813 - Move all mapping related code to mappings package. #2822

2813 - Move all mapping related code to mappings package. #2822

Conversation

bhiravabhatla commented Feb 22, 2021 • edited Loading

Which problem is this PR solving?

Short description of the changes

bhiravabhatla commented Feb 22, 2021

bhiravabhatla commented Feb 22, 2021

codecov bot commented Feb 22, 2021 • edited Loading

Codecov Report

bhiravabhatla commented Feb 22, 2021

bhiravabhatla commented Feb 22, 2021

bhiravabhatla commented Feb 22, 2021

albertteoh commented Feb 22, 2021 • edited Loading

bhiravabhatla commented Feb 24, 2021

yurishkuro Feb 25, 2021

Choose a reason for hiding this comment

bhiravabhatla Feb 26, 2021

Choose a reason for hiding this comment

yurishkuro Feb 26, 2021

Choose a reason for hiding this comment

bhiravabhatla Feb 26, 2021

Choose a reason for hiding this comment

albertteoh Feb 25, 2021

Choose a reason for hiding this comment

bhiravabhatla Feb 26, 2021

Choose a reason for hiding this comment

yurishkuro Feb 26, 2021

Choose a reason for hiding this comment

bhiravabhatla commented Feb 22, 2021 •

edited

Loading

codecov bot commented Feb 22, 2021 •

edited

Loading

albertteoh commented Feb 22, 2021 •

edited

Loading