Skip to content
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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

bhiravabhatla
Copy link
Contributor

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

@bhiravabhatla bhiravabhatla requested a review from a team as a code owner August 22, 2023 13:43
@codecov
Copy link

codecov bot commented Aug 22, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (f1658ed) 97.04% compared to head (e6691b9) 97.04%.

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           
Flag Coverage Δ
unittests 97.04% <100.00%> (+<0.01%) ⬆️

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

Files Changed Coverage Δ
plugin/storage/es/mappings/mapping.go 100.00% <ø> (ø)
cmd/es-rollover/app/flags.go 100.00% <100.00%> (ø)
cmd/es-rollover/app/init/action.go 93.81% <100.00%> (+0.06%) ⬆️
cmd/esmapping-generator/app/flags.go 100.00% <100.00%> (ø)
cmd/esmapping-generator/app/renderer/render.go 100.00% <100.00%> (ø)
plugin/storage/es/factory.go 97.67% <100.00%> (+0.01%) ⬆️
plugin/storage/es/mappings/fieldType.go 100.00% <100.00%> (ø)
plugin/storage/es/options.go 98.55% <100.00%> (+0.03%) ⬆️
plugin/storage/es/spanstore/reader.go 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

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

Comment on lines 280 to 281
"(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.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"(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) {
Copy link
Member

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 {
Copy link
Member

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
)
Copy link
Member

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
Copy link
Member

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"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
type FieldType int
type FieldMappingType string
const (
FieldMappingObject = "object"
FieldMappingNested = "nested"
)

@@ -0,0 +1,59 @@
// Copyright (c) 2023 The Jaeger Authors.
Copy link
Member

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" : {}
},
Copy link
Member

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 @@
{
Copy link
Member

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 @@
{
Copy link
Member

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

@bhiravabhatla
Copy link
Contributor Author

@yurishkuro thanks for the review. I will spend sometime over weekend and revert on the comments.

@pc-coder
Copy link

pc-coder commented Sep 8, 2024

@yurishkuro @wasup-yash We have refactored the code and made the requested changes. Can you please take a look?

bhiravabhatla and others added 10 commits September 20, 2024 16:01
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants