feat: Add esmapping-generator into jaeger binary #6327
+62
−4
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@yurishkuro I think now this is perfectly decoupling the logic for generation in tool and then calling it from jaeger binary. Looking forward to your suggestions! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6327 +/- ##
===========================================
- Coverage 96.27% 50.21% -46.07%
===========================================
Files 372 188 -184
Lines 21283 11405 -9878
===========================================
- Hits 20491 5727 -14764
- Misses 605 5220 +4615
- Partials 187 458 +271
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
yurishkuro
reviewed
Jan 10, 2025
| "go.uber.org/zap" | ||
| ) | ||
|
|
||
| func GenerateESMappings(logger *zap.Logger, options app.Options) error { |
There was a problem hiding this comment.
- this can be in the app/ package
- can be called GenerateMappings ("ES" is implied)
- as a library function it needs to return the results, not print them. The caller can decide what to do with the string.
|
|
||
| fmt.Println(parsedMapping) | ||
| return nil | ||
| } |
| @@ -0,0 +1,26 @@ | |||
| package es | |||
There was a problem hiding this comment.
this package is not a place for commands. Can move it to cmd/esmapping-generator/app/command.go
| Short: "Generate Elasticsearch mappings", | ||
| Long: "Generate Elasticsearch mappings using jaeger-esmapping-generator functionality", | ||
| Run: func(_ *cobra.Command, _ []string) { | ||
| if err := renderer.GenerateESMappings(logger, options); err != nil { |
cmd/esmapping-generator/main.go
Outdated
| @@ -25,19 +23,13 @@ func main() { | |||
| Short: "Jaeger esmapping-generator prints rendered mappings as string", | |||
| Long: `Jaeger esmapping-generator renders passed templates with provided values and prints rendered output to stdout`, | |||
| Run: func(_ *cobra.Command, _ /* args */ []string) { | |||
| if _, err := mappings.MappingTypeFromString(options.Mapping); err != nil { | |||
There was a problem hiding this comment.
how is this command different from the new one you defined? Merge into one?
5 tasks
Signed-off-by: Rohanraj123 <rajrohan88293@gmail.com>
Rohanraj123
force-pushed
the
esmapping
branch
from
24c7af6 to
01ad9bc
Compare
yurishkuro
reviewed
Jan 11, 2025
| @@ -199,6 +200,7 @@ by default uses only in-memory database.`, | |||
| command.AddCommand(docs.Command(v)) | |||
| command.AddCommand(status.Command(v, ports.CollectorAdminHTTP)) | |||
| command.AddCommand(printconfig.Command(v)) | |||
| command.AddCommand(esmapping.Command(svc.Logger)) | |||
There was a problem hiding this comment.
no need to add this to v1 binaries, only cmd/jaeger
| "github.com/jaegertracing/jaeger/cmd/esmapping-generator/app" | ||
| "github.com/jaegertracing/jaeger/cmd/esmapping-generator/app/renderer" | ||
| "github.com/jaegertracing/jaeger/pkg/es" | ||
| "github.com/jaegertracing/jaeger/pkg/version" | ||
| "github.com/jaegertracing/jaeger/plugin/storage/es/mappings" | ||
| "github.com/spf13/cobra" | ||
| "go.uber.org/zap" |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which problem is this PR solving?
Description of the changes
How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:yarn lintandyarn test