Add 'features' command to beautify and print feature gates #6542
Conversation
Signed-off-by: cs-308-2023 <[email protected]>
Signed-off-by: cs-308-2023 <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6542 +/- ##
==========================================
+ Coverage 50.21% 50.27% +0.05%
==========================================
Files 188 189 +1
Lines 11405 11424 +19
==========================================
+ Hits 5727 5743 +16
- Misses 5220 5226 +6
+ Partials 458 455 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: cs-308-2023 <[email protected]>
Signed-off-by: cs-308-2023 <[email protected]>
Signed-off-by: cs-308-2023 <[email protected]>
cmd/jaeger/internal/command.go
Outdated
| @@ -26,6 +26,10 @@ var yamlAllInOne embed.FS | |||
|
|
|||
| const description = "Jaeger backend v2" | |||
|
|
|||
| func GetDescription() string { | |||
There was a problem hiding this comment.
It is needed because in the file features/main.go, there is a need to print the current version of the backend. For that, I have used the following
The other way around was to change the variable name from description to "Description" to export the variable, but that would require making changes in a lot of places.
cmd/jaeger/features/main.go
Outdated
|
|
||
| fmt.Println("\n" + internal.GetDescription()) | ||
| fmt.Println("List of feature gate identifiers. Prefix with '-' to disable the feature. '+' or no prefix will enable the feature.\n ") | ||
| otelcol.FeatureCommand(settings) |
There was a problem hiding this comment.
if there is going to be a feature subcommand in OTEL why do we need any of this? We can just invoke it and add to our root command.
There was a problem hiding this comment.
Okay. So, I will move the fmt.Println("List of feature gate identifiers. Prefix with '-' to disable the feature. '+' or no prefix will enable the feature.\n ") inside the FeatureFlags function but
fmt.Println("\n" + internal.GetDescription()) has to be in this file only because it will display that it is a Jaeger Backend v2
There was a problem hiding this comment.
The output of the (sub)-command is completed determined by the command itself, it does not need to borrow description from anywhere else. For example, version subcommand only outputs a JSON:
$ go run ./cmd/jaeger version
2025/01/20 11:11:45 application version: git-commit=, git-version=, build-date=
{"gitCommit":"","gitVersion":"","buildDate":""}%
(we need to clean up the first log line to not be printed)
Signed-off-by: cs-308-2023 <[email protected]>
Signed-off-by: cs-308-2023 <[email protected]>
|
|
||
| fmt.Println("\n" + internal.GetDescription()) | ||
| fmt.Println("List of feature gate identifiers. Prefix with '-' to disable the feature. '+' or no prefix will enable the feature.\n ") | ||
| otelcol.FeatureCommand(settings) |
There was a problem hiding this comment.
why does otelcol.FeatureCommand need any of the above? It seems completely irrelevant.
Suggestion: copy the core logic of FeatureCommand into Jaeger directly for now, so that we don't have to wait for OTEL Collector reease.
Signed-off-by: cs-308-2023 <[email protected]>
| // Copyright (c) 2023 The Jaeger Authors. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| package features |
There was a problem hiding this comment.
cmd/jaeger/internal/features/display.go
cmd/jaeger/features/display.go
Outdated
| return | ||
| } | ||
| f.reg.VisitAll(func(g *featuregate.Gate) { | ||
| str := "Feature:\t" |
There was a problem hiding this comment.
use exact number of spaces, not \t
There was a problem hiding this comment.
\t is giving me beautified output
|
For the unit test, I am thinking the following:
|
Signed-off-by: cs-308-2023 <[email protected]>
cmd/jaeger/features/display.go
Outdated
| str := "Feature:\t" | ||
| id := g.ID() | ||
| desc := g.Description() | ||
| str += id + "\n" |
There was a problem hiding this comment.
why are you concatenating strings instead of just printing?
There was a problem hiding this comment.
Otherwise, I had to write fmt.Println() lot of time and it would be too big of a code, so I thought it would be better to concatenate and then print in the end
Signed-off-by: cs-308-2023 <[email protected]>
Which problem is this PR solving?
Description of the changes
How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:npm run lintandnpm run test