-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
refactor: use enum instead of strings for ES mappings #6068
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Saumya Shah <saumyabshah90@gmail.com>
f2b6b51
to
ac6de11
Compare
Signed-off-by: Saumya Shah <saumyabshah90@gmail.com>
Signed-off-by: Saumya Shah <saumyabshah90@gmail.com>
Signed-off-by: Saumya Shah <saumyabshah90@gmail.com>
Signed-off-by: Saumya Shah <saumyabshah90@gmail.com>
anything im missing out on here @yurishkuro ? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6068 +/- ##
==========================================
- Coverage 96.92% 96.81% -0.12%
==========================================
Files 349 349
Lines 16598 16650 +52
==========================================
+ Hits 16087 16119 +32
- Misses 328 345 +17
- Partials 183 186 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
{mapping: SpanMapping, esVersion: 6}, | ||
{mapping: ServiceMapping, esVersion: 8}, | ||
{mapping: ServiceMapping, esVersion: 7}, | ||
{mapping: DependenciesMapping, esVersion: 6}, |
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.
{mapping: DependenciesMapping, esVersion: 6}, | |
{mapping: ServiceMapping, esVersion: 6}, |
case SamplingMapping: | ||
return "jaeger-sampling", nil | ||
default: | ||
return "", fmt.Errorf("Unknown mapping type %d", mt) |
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.
String() methods usually do not return errors. You can return "unknown"
string.
return ok | ||
} | ||
|
||
// stringToMappingType converts a string to a MappingType | ||
func stringToMappingType(val string) (mappings.MappingType, error) { |
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.
this translation can be co-located with the MappingType
definition:
func MappingTypeFromString(val string) (MappingType, error)
"jaeger-service": {}, | ||
"jaeger-dependencies": {}, | ||
"jaeger-sampling": {}, | ||
var supportedMappings = map[mappings.MappingType]struct{}{ |
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.
os this still needed?
return "", err | ||
} | ||
|
||
return mappingBuilder.GetMapping(mappingType) | ||
} | ||
|
||
// IsValidOption checks if passed option is a valid index template. | ||
func IsValidOption(val string) bool { |
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 think this function is needed. Wherever it's called the called can instead do
if m, err := MappingTypeFromString(val); err == nil {
//
}
Which problem is this PR solving?
Description of the changes
plugin/storage/es/mappings/mapping.go
we are using bare string inorder to for the file mapping. This PR refractors this approach to use enums instead of stringHow was this change tested?
go test
Checklist
jaeger
:make lint test
jaeger-ui
:yarn lint
andyarn test