-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
3085 - Change type of logs from nested to object to reduce fixedbit memory consumption #4697
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these should go to the top, next to type declaration
"strconv" | ||
) | ||
|
||
type FieldType int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do this as int
, instead of string
with the expected values of "object"
and "nested"
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this changing in this PR?
@@ -0,0 +1,17 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please explain motivation for adding this. How is your PR related to ILM ?
@@ -0,0 +1,48 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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 <jskswamy@gmail.com> Signed-off-by: Parvesh Chaudhary <parveshchaudhary111@gmail.com>
Templatize logs.fields mapping to allow user to choose not to have nested type. Co-authored-by: Krishnaswamy Subramanian <jskswamy@gmail.com> Signed-off-by: Parvesh Chaudhary <parveshchaudhary111@gmail.com>
Co-authored-by: Krishnaswamy Subramanian <jskswamy@gmail.com> Signed-off-by: Parvesh Chaudhary <parveshchaudhary111@gmail.com>
Co-authored-by: Krishnaswamy Subramanian <jskswamy@gmail.com> Signed-off-by: Parvesh Chaudhary <parveshchaudhary111@gmail.com>
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 <jskswamy@gmail.com> Signed-off-by: Parvesh Chaudhary <parveshchaudhary111@gmail.com>
Co-authored-by: Krishnaswamy Subramanian <jskswamy@gmail.com> Signed-off-by: Parvesh Chaudhary <parveshchaudhary111@gmail.com>
Signed-off-by: Parvesh Chaudhary <parveshchaudhary111@gmail.com>
Signed-off-by: Parvesh Chaudhary <parveshchaudhary111@gmail.com>
Co-authored-by: Raghav Gupta <raghav1674> Signed-off-by: Parvesh Chaudhary <parveshchaudhary111@gmail.com>
…mplate Co-authored-by: Raghav Gupta <ragaws1674@gmail.com> Signed-off-by: Parvesh Chaudhary <parveshchaudhary111@gmail.com>
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 test
jaeger-ui
:yarn lint
andyarn test