3085 - Change type of logs from nested to object to reduce fixedbit memory consumption #4697
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #4697 +/- ##
=======================================
Coverage 97.04% 97.04%
=======================================
Files 301 302 +1
Lines 17880 17925 +45
=======================================
+ Hits 17352 17396 +44
- Misses 423 424 +1
Partials 105 105
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
plugin/storage/es/options.go
Outdated
| "(experimental) Option to disable search on logs.field field of jaeger span index. Setting this option to true would set mapping of "+ | ||
| "logs.field field span index to object type. Its set to false by default.", |
There was a problem hiding this comment.
| "(experimental) Option to disable search on logs.field field of jaeger span index. Setting this option to true would set mapping of "+ | |
| "logs.field field span index to object type. Its set to false by default.", | |
| "(experimental) When true, turns off the ability to search within log fields, and uses " + | |
| "more efficient 'object' mapping for logs[].fields[], instead of 'nested' mapping.", |
I recommend putting this string into a constant and reusing across all binaries involved, so that we have consistent description.
| return NestedFieldType | ||
| } | ||
|
|
||
| func (field FieldType) Format(f fmt.State, verb rune) { |
There was a problem hiding this comment.
which interface does this implement? Please add a check, e.g.
var _ ExpectedInterface = (*MyType)(nil)
|
|
||
| type FieldType int | ||
|
|
||
| func ParseFieldType(v any) FieldType { |
There was a problem hiding this comment.
I don't understand the purpose of this function, please document it. It seems like you're using it by passing a Boolean value of the CLI option, which doesn't make sense to me - it's fine to declare enums in this module, but the mapping from CLI flag to the mapping type should be done close to where the CLI flag is defined. I also think that instead of pushing down DisableLogsFieldSearch flag all the way down to storage, you could've instead declared LogsFieldsMappingType parameter in the storage, and map boolean CLI flag to one of the values.
| const ( | ||
| NestedFieldType FieldType = iota | ||
| ObjectFieldType | ||
| ) |
There was a problem hiding this comment.
these should go to the top, next to type declaration
| "strconv" | ||
| ) | ||
|
|
||
| type FieldType int |
There was a problem hiding this comment.
why do this as int, instead of string with the expected values of "object" and "nested"?
There was a problem hiding this comment.
| type FieldType int | |
| type FieldMappingType string | |
| const ( | |
| FieldMappingObject = "object" | |
| FieldMappingNested = "nested" | |
| ) |
| @@ -0,0 +1,59 @@ | |||
| // Copyright (c) 2023 The Jaeger Authors. | |||
There was a problem hiding this comment.
rename file to mapping_type.go, "field type" does not match ES nomenclature
| @@ -1,17 +1,10 @@ | |||
| { | |||
| "index_patterns": "*jaeger-dependencies-*", | |||
| "aliases": { | |||
| "test-jaeger-dependencies-read" : {} | |||
| }, | |||
There was a problem hiding this comment.
why is this changing in this PR?
| @@ -0,0 +1,17 @@ | |||
| { | |||
There was a problem hiding this comment.
please explain motivation for adding this. How is your PR related to ILM ?
| @@ -0,0 +1,48 @@ | |||
| { | |||
There was a problem hiding this comment.
how different is it from -7? I would rather use some template / substitution, not duplicate a pretty large file
|
@yurishkuro thanks for the review. I will spend sometime over weekend and revert on the comments. |
pc-coder
force-pushed
the
3085
branch
3 times, most recently
from
5793145
to
47f8904
Compare
|
@yurishkuro @wasup-yash We have refactored the code and made the requested changes. Can you please take a look? |
Add testcase for useILM and pass fixture filename as test field Co-authored-by: Krishnaswamy Subramanian <[email protected]> Signed-off-by: Parvesh Chaudhary <[email protected]>
Templatize logs.fields mapping to allow user to choose not to have nested type. Co-authored-by: Krishnaswamy Subramanian <[email protected]> Signed-off-by: Parvesh Chaudhary <[email protected]>
Co-authored-by: Krishnaswamy Subramanian <[email protected]> Signed-off-by: Parvesh Chaudhary <[email protected]>
Co-authored-by: Krishnaswamy Subramanian <[email protected]> Signed-off-by: Parvesh Chaudhary <[email protected]>
Setting disableLogsFieldSearch flag to true would make logs.fields field in es index non-searchable. This is needed to reduce the memory taken by logs.fields when they are searchable and are of type nested. Co-authored-by: Krishnaswamy Subramanian <[email protected]> Signed-off-by: Parvesh Chaudhary <[email protected]>
Co-authored-by: Krishnaswamy Subramanian <[email protected]> Signed-off-by: Parvesh Chaudhary <[email protected]>
Signed-off-by: Parvesh Chaudhary <[email protected]>
Signed-off-by: Parvesh Chaudhary <[email protected]>
Co-authored-by: Raghav Gupta <raghav1674> Signed-off-by: Parvesh Chaudhary <[email protected]>
…mplate Co-authored-by: Raghav Gupta <[email protected]> Signed-off-by: Parvesh Chaudhary <[email protected]>
Which problem is this PR solving?
Resolves #3085
Description of the changes
Adds a flag to disable search on logs field and changes the type of logs from nested to object to reduce fixedbit memory consumption.
How was this change tested?
Added required unit tests. Tested in one of our project's lower environment end to end.
Checklist
jaeger:make lint testjaeger-ui:yarn lintandyarn test