feat: Add esmapping-generator into jaeger binary #6327

Rohanraj123 · 2024-12-08T18:36:22Z

Which problem is this PR solving?

part of Remove the need for external tools for managing Elasticsearch #6283

Description of the changes

Decoupled the Esmapping generation logic from cmd/esmappnig-generator/main.go and kept it into reusable renderer package and then reimplemented the subcommand in the plugin/storage/es/esmapping-generator.go file and then registered in jaeger/main.go.

How was this change tested?

go test ./... -count=1

Checklist

I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
I have signed all commits
I have added unit tests for the new functionality
I have run lint and test steps successfully
- for jaeger: make lint test
- for jaeger-ui: yarn lint and yarn test

Rohanraj123 · 2024-12-08T18:38:01Z

@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 · 2024-12-08T18:44:10Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 50.30%. Comparing base (980dc31) to head (7ae6c2e).
Report is 20 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (980dc31) and HEAD (7ae6c2e). Click for more details.

HEAD has 1 upload less than BASE

Flag BASE (980dc31) HEAD (7ae6c2e)

unittests 1 0

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #6327       +/-   ##
===========================================
- Coverage   96.27%   50.30%   -45.98%     
===========================================
  Files         372      188      -184     
  Lines       21283    11411     -9872     
===========================================
- Hits        20491     5740    -14751     
- Misses        605     5217     +4612     
- Partials      187      454      +267

Flag	Coverage Δ
badger_v1	`10.65% <ø> (-0.02%)`	⬇️
badger_v2	`2.78% <ø> (-0.01%)`	⬇️
cassandra-4.x-v1-manual	`16.55% <ø> (-0.03%)`	⬇️
cassandra-4.x-v2-auto	`2.71% <ø> (-0.01%)`	⬇️
cassandra-4.x-v2-manual	`2.71% <ø> (-0.01%)`	⬇️
cassandra-5.x-v1-manual	`16.55% <ø> (-0.03%)`	⬇️
cassandra-5.x-v2-auto	`2.71% <ø> (-0.01%)`	⬇️
cassandra-5.x-v2-manual	`2.74% <ø> (+0.02%)`	⬆️
elasticsearch-6.x-v1	`20.34% <ø> (+0.10%)`	⬆️
elasticsearch-7.x-v1	`20.41% <ø> (+0.10%)`	⬆️
elasticsearch-8.x-v1	`20.57% <ø> (+0.11%)`	⬆️
elasticsearch-8.x-v2	`2.77% <ø> (-0.01%)`	⬇️
grpc_v1	`12.17% <ø> (-0.15%)`	⬇️
grpc_v2	`9.04% <ø> (-0.06%)`	⬇️
kafka-3.x-v1	`10.33% <ø> (-0.02%)`	⬇️
kafka-3.x-v2	`2.78% <ø> (-0.01%)`	⬇️
memory_v2	`2.77% <ø> (-0.01%)`	⬇️
opensearch-1.x-v1	`20.46% <ø> (+0.11%)`	⬆️
opensearch-2.x-v1	`20.46% <ø> (+0.10%)`	⬆️
opensearch-2.x-v2	`2.78% <ø> (-0.01%)`	⬇️
tailsampling-processor	`0.51% <ø> (-0.01%)`	⬇️
unittests	`?`

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

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

yurishkuro · 2024-12-08T19:45:59Z

cmd/esmapping-generator/app/renderer/mapping_generator.go

+	"go.uber.org/zap"
+)
+
+func GenerateESMappings(logger *zap.Logger, options app.Options) error {


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.

yurishkuro · 2024-12-08T19:46:12Z

cmd/esmapping-generator/app/renderer/mapping_generator.go

+
+	fmt.Println(parsedMapping)
+	return nil
+}


needs tests

yurishkuro · 2024-12-08T19:47:44Z

plugin/storage/es/esmapping-generator.go

@@ -0,0 +1,26 @@
+package es


this package is not a place for commands. Can move it to cmd/esmapping-generator/app/command.go

yurishkuro · 2024-12-08T19:48:02Z

plugin/storage/es/esmapping-generator.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 {


where are options populated?

cmd/esmapping-generator/main.go

yurishkuro

how is a user expected to run this?

yurishkuro · 2025-01-11T22:38:42Z

cmd/all-in-one/main.go

@@ -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))


no need to add this to v1 binaries, only cmd/jaeger

yurishkuro · 2025-01-11T22:39:02Z

cmd/esmapping-generator/main.go

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


run make fmt

`:~/jaeger$ ./jaeger es-mappings --mapping jaeger-span 2025/01/12 17:15:13 application version: git-commit=, git-version=, build-date= Generating mappings with options: {Mapping:jaeger-span EsVersion:7 Shards:5 Replicas:1 IndexPrefix:jaeger UseILM:false ILMPolicyName:} { "index_patterns": "*jaeger-jaeger-span-*", "settings":{ "index.number_of_shards": 5, "index.number_of_replicas": 1, "index.mapping.nested_fields.limit":50, "index.requests.cache.enable":true }, "mappings":{ "dynamic_templates":[ { "span_tags_map":{ "mapping":{ "type":"keyword", "ignore_above":256 }, "path_match":"tag.*" } }, { "process_tags_map":{ "mapping":{ "type":"keyword", "ignore_above":256 }, "path_match":"process.tag.*" } } ], "properties":{ "traceID":{ "type":"keyword", "ignore_above":256 }, "parentSpanID":{ "type":"keyword", "ignore_above":256 }, "spanID":{ "type":"keyword", "ignore_above":256 }, "operationName":{ "type":"keyword", "ignore_above":256 }, "startTime":{ "type":"long" }, "startTimeMillis":{ "type":"date", "format":"epoch_millis" }, "duration":{ "type":"long" }, "flags":{ "type":"integer" }, "logs":{ "type":"nested", "dynamic":false, "properties":{ "timestamp":{ "type":"long" }, "fields":{ "type":"nested", "dynamic":false, "properties":{ "key":{ "type":"keyword", "ignore_above":256 }, "value":{ "type":"keyword", "ignore_above":256 }, "type":{ "type":"keyword", "ignore_above":256 } } } } }, "process":{ "properties":{ "serviceName":{ "type":"keyword", "ignore_above":256 }, "tag":{ "type":"object" }, "tags":{ "type":"nested", "dynamic":false, "properties":{ "key":{ "type":"keyword", "ignore_above":256 }, "value":{ "type":"keyword", "ignore_above":256 }, "type":{ "type":"keyword", "ignore_above":256 } } } } }, "references":{ "type":"nested", "dynamic":false, "properties":{ "refType":{ "type":"keyword", "ignore_above":256 }, "traceID":{ "type":"keyword", "ignore_above":256 }, "spanID":{ "type":"keyword", "ignore_above":256 } } }, "tag":{ "type":"object" }, "tags":{ "type":"nested", "dynamic":false, "properties":{ "key":{ "type":"keyword", "ignore_above":256 }, "value":{ "type":"keyword", "ignore_above":256 }, "type":{ "type":"keyword", "ignore_above":256 } } } } } } rohan@rohan-IdeaPad-3-15IIL05-Ua:~/jaeger$ `

This is how I generated esmapping after building jaeger binary. I needed to add some extra flag values.

Signed-off-by: Rohanraj123 <[email protected]>

cmd/esmapping-generator/generator/esmapping_generator.go

Signed-off-by: Rohanraj123 <[email protected]>

yurishkuro · 2025-01-15T23:15:07Z

internal/esmapping/command.go

+	}
+
+	options.AddFlags(command)
+	command.AddCommand(version.Command())


yurishkuro · 2025-01-15T23:36:19Z

cmd/esmapping-generator/main.go

 )

 func main() {
-	logger, _ := zap.NewDevelopment()
 	options := app.Options{}
 	command := &cobra.Command{


Why this cannot be the same command as the one you introduce in internal?

You can override its Use: field to maintain the old name, everything else is the same

I did the same. I added the central command to the esmapping-generator/main.go file

Signed-off-by: Rohanraj123 <[email protected]>

yurishkuro · 2025-01-16T01:56:58Z

cmd/esmapping-generator/generator/esmapping_generator.go

+	"github.com/jaegertracing/jaeger/plugin/storage/es/mappings"
+)
+
+func GenerateMappings(options app.Options) string {


Suggested change

func GenerateMappings(options app.Options) string {

func GenerateMappings(options app.Options) (string, error) {

return proper errors and log them from the caller

yurishkuro · 2025-01-16T01:57:46Z

internal/esmapping/command.go

+	"github.com/jaegertracing/jaeger/cmd/esmapping-generator/generator"
+)
+
+func Command() *cobra.Command {


please add tests and run make lint (which will fail right now)

Signed-off-by: Rohanraj123 <[email protected]>

yurishkuro · 2025-01-16T14:31:24Z

cmd/esmapping-generator/main.go

-
-	command.AddCommand(version.Command())
+	esmappingsCmd.Use = rootCmd.Use
+	rootCmd.AddCommand(esmappingsCmd)


This is not equivalent. You need to use the command returned from helper AS the root command.

If we treat the returned command as a root command then we will not get any specific command as we were getting before. Example of what I mean :
Before :
I use this command to run the command :
go build -o jaeger-esmapping-generator . to build
./jaeger-esmapping-generator es-mappings --mapping=jaeger-service --es-version=7 --shards=5 --replicas=1 --index-prefix=jaeger-service-index --use-ilm=true --ilm-policy-name=service-ilm-policy to run the command

After :
go build -o jaeger-esmapping-generator . to build
./jaeger-esmapping-generator --mapping=jaeger-service --es-version=7 --shards=5 --replicas=1 --index-prefix=jaeger-service-index --use-ilm=true --ilm-policy-name=service-ilm-policy to run the command.

I didnt find any way of overriding that previous command that was only possible when I add the returned command to some root command. Please let me know if there any workaround.

The existing binary does not implement any subcommand to execute the functionality

$ go run ./cmd/esmapping-generator help Jaeger esmapping-generator renders passed templates with provided values and prints rendered output to stdout Usage: jaeger-esmapping-generator [flags] jaeger-esmapping-generator [command] Available Commands: completion Generate the autocompletion script for the specified shell help Help about any command version Print the version.

Neither should it have subcommands after this change.

yurishkuro · 2025-01-16T14:35:58Z

internal/esmapping/command_test.go

+
+	capturedOutput := output.String()
+
+	expectedOutput := `{


The command itself is not responsible for the content of the mapping produced by the rendered (and presumably already tested for content). The tests here should only validate the sanity of the output, the comma-to-comma comparison. The same applies to the generator test.

So basically I need to just check whether the output is valid Json or not . Also whether the flags are present in the output also in a valid format or not. And error messages should be correctly handlled . Please correct me If I am wrong. Then I will update tests like that.

Rohanraj123 marked this pull request as ready for review December 8, 2024 18:36

Rohanraj123 requested a review from a team as a code owner December 8, 2024 18:36

Rohanraj123 requested a review from mahadzaryab1 December 8, 2024 18:36

dosubot bot added the area/storage label Dec 8, 2024

yurishkuro reviewed Jan 10, 2025

View reviewed changes

yurishkuro mentioned this pull request Jan 10, 2025

Remove the need for external tools for managing Elasticsearch #6283

Open

5 tasks

Rohanraj123 force-pushed the esmapping branch from 24c7af6 to 01ad9bc Compare January 11, 2025 17:47

yurishkuro reviewed Jan 11, 2025

View reviewed changes

Add esmapping-generator to jaeger binary

be85f38

Signed-off-by: Rohanraj123 <[email protected]>

Rohanraj123 force-pushed the esmapping branch from 01ad9bc to be85f38 Compare January 12, 2025 12:03

Rohanraj123 added 4 commits January 12, 2025 19:41

Add tests for that

3206ea2

Signed-off-by: Rohanraj123 <[email protected]>

Cleaned the code

16ac93c

Signed-off-by: Rohanraj123 <[email protected]>

Run make fmt

cf41bf3

Signed-off-by: Rohanraj123 <[email protected]>

Remove logger from the argument in Command()

c460e7b

Signed-off-by: Rohanraj123 <[email protected]>

yurishkuro reviewed Jan 12, 2025

View reviewed changes

cmd/esmapping-generator/generator/esmapping_generator.go Outdated Show resolved Hide resolved

Added GenerateMappings() method in main.go file

6d52a6e

Signed-off-by: Rohanraj123 <[email protected]>

yurishkuro reviewed Jan 15, 2025

View reviewed changes

Rohanraj123 added 2 commits January 16, 2025 07:08

Add the central command to esmapping-generator/main.go file

1576089

Signed-off-by: Rohanraj123 <[email protected]>

Restructured the code

16cac79

Signed-off-by: Rohanraj123 <[email protected]>

yurishkuro reviewed Jan 16, 2025

View reviewed changes

Rohanraj123 added 2 commits January 16, 2025 11:30

Add tests for internal/command.go file

5877e85

Signed-off-by: Rohanraj123 <[email protected]>

Removed exe files

7ae6c2e

Signed-off-by: Rohanraj123 <[email protected]>

yurishkuro reviewed Jan 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add esmapping-generator into jaeger binary #6327

feat: Add esmapping-generator into jaeger binary #6327

Rohanraj123 commented Dec 8, 2024 •

edited

Loading

Rohanraj123 commented Dec 8, 2024

codecov bot commented Dec 8, 2024 •

edited

Loading

yurishkuro Dec 8, 2024

yurishkuro Dec 8, 2024

yurishkuro Dec 8, 2024

yurishkuro Dec 8, 2024

yurishkuro left a comment

yurishkuro Jan 11, 2025

yurishkuro Jan 11, 2025

Rohanraj123 Jan 12, 2025 •

edited

Loading

yurishkuro Jan 15, 2025

yurishkuro Jan 15, 2025

Rohanraj123 Jan 16, 2025

yurishkuro Jan 16, 2025

yurishkuro Jan 16, 2025

yurishkuro Jan 16, 2025

Rohanraj123 Jan 17, 2025

yurishkuro Jan 18, 2025

yurishkuro Jan 16, 2025

Rohanraj123 Jan 17, 2025

	func GenerateMappings(options app.Options) string {
	func GenerateMappings(options app.Options) (string, error) {

feat: Add esmapping-generator into jaeger binary #6327

Are you sure you want to change the base?

feat: Add esmapping-generator into jaeger binary #6327

Conversation

Rohanraj123 commented Dec 8, 2024 • edited Loading

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

Rohanraj123 commented Dec 8, 2024

codecov bot commented Dec 8, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rohanraj123 Jan 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rohanraj123 commented Dec 8, 2024 •

edited

Loading

codecov bot commented Dec 8, 2024 •

edited

Loading

Rohanraj123 Jan 12, 2025 •

edited

Loading