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.

Sign up for GitHub

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

Jump to bottom

Add 'features' command to print available feature gates #6542

Merged
merged 11 commits into from
Jan 26, 2025
Merged

Add 'features' command to print available feature gates #6542

merged 11 commits into from
Jan 26, 2025
+88 −0

Conversation

ADI-ROXX
Copy link
Contributor

@ADI-ROXX ADI-ROXX commented Jan 14, 2025 •
edited
Loading

Which problem is this PR solving?

Description of the changes

How was this change tested?

  • make features

Checklist

@ADI-ROXX ADI-ROXX requested a review from a team as a code owner January 14, 2025 04:30
@ADI-ROXX ADI-ROXX requested a review from pavolloffay January 14, 2025 04:30
@dosubot dosubot bot added the enhancement label Jan 14, 2025
@codecov Codecov
Copy link

codecov bot commented Jan 14, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.03%. Comparing base (6d4d7c4) to head (ca4db7e).
Report is 2 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #6542      +/-   ##
==========================================
- Coverage   96.05%   96.03%   -0.03%     
==========================================
  Files         364      365       +1     
  Lines       20750    20767      +17     
==========================================
+ Hits        19932    19944      +12     
- Misses        622      626       +4     
- Partials      196      197       +1
Flag Coverage Δ
badger_v1 9.88% <ø> (ø)
badger_v2 1.83% <ø> (ø)
cassandra-4.x-v1-manual 15.02% <ø> (ø)
cassandra-4.x-v2-auto 1.82% <ø> (ø)
cassandra-4.x-v2-manual 1.82% <ø> (ø)
cassandra-5.x-v1-manual 15.02% <ø> (ø)
cassandra-5.x-v2-auto 1.82% <ø> (ø)
cassandra-5.x-v2-manual 1.82% <ø> (ø)
elasticsearch-6.x-v1 19.23% <ø> (ø)
elasticsearch-7.x-v1 19.31% <ø> (ø)
elasticsearch-8.x-v1 19.48% <ø> (ø)
elasticsearch-8.x-v2 1.83% <ø> (ø)
grpc_v1 11.18% <ø> (ø)
grpc_v2 8.04% <ø> (ø)
kafka-3.x-v1 10.17% <ø> (ø)
kafka-3.x-v2 1.83% <ø> (ø)
memory_v2 1.83% <ø> (ø)
opensearch-1.x-v1 19.36% <ø> (ø)
opensearch-2.x-v1 19.36% <ø> (ø)
opensearch-2.x-v2 1.83% <ø> (ø)
tailsampling-processor 0.48% <ø> (ø)
unittests 94.82% <100.00%> (-0.02%) ⬇️

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
yurishkuro reviewed Jan 15, 2025
View reviewed changes
Makefile Outdated Show resolved Hide resolved
cmd/jaeger/features/main.go Outdated Show resolved Hide resolved
yurishkuro
yurishkuro reviewed Jan 19, 2025
View reviewed changes
Makefile Outdated Show resolved Hide resolved
yurishkuro
yurishkuro reviewed Jan 19, 2025
View reviewed changes
cmd/jaeger/features/main.go Outdated Show resolved Hide resolved
cmd/jaeger/internal/command.go Outdated Show resolved Hide resolved
cmd/jaeger/main.go Outdated Show resolved Hide resolved
cmd/jaeger/features/main.go Outdated Show resolved Hide resolved
yurishkuro
yurishkuro reviewed Jan 20, 2025
View reviewed changes
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

please add a unit test

@ADI-ROXX
Copy link
Contributor Author

ADI-ROXX commented Jan 20, 2025 •
edited
Loading

For the unit test, I am thinking the following:

  1. In the DisplayFeatures() function, return a string.
  2. Set a desired string. Check if the string output from DisplayFeatures is same as the desired output

Is this methodology correct? Otherwise, can you suggest a suitable testing mechanism?

@ADI-ROXX
Beautified --feature-flags
53ca041 
Signed-off-by: cs-308-2023 <[email protected]>
yurishkuro
yurishkuro reviewed Jan 25, 2025
View reviewed changes
cmd/jaeger/internal/features/display_test.go Outdated
}

func TestDisplay(t *testing.T) {
internal.Command()
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without running internal.Command(), featuregate.GlobalRegistry() returns an empty array, so the output string is empty.

After running internal.Command(), I am able to get the flags.

@yurishkuro
Copy link
Member

if you are not getting any features during unit test, you can register a fake one just for the test, similar to this:

jaeger/cmd/jaeger/internal/extension/remotesampling/config.go

Lines 31 to 38 in 5c9f16d

includeDefaultOpStrategies = featuregate.GlobalRegistry().MustRegister(
"jaeger.sampling.includeDefaultOpStrategies",
featuregate.StageBeta, // enabed by default
featuregate.WithRegisterFromVersion("v2.2.0"),
featuregate.WithRegisterToVersion("v2.5.0"),
featuregate.WithRegisterDescription("Forces service strategy to be merged with default strategy, including per-operation overrides."),
featuregate.WithRegisterReferenceURL("https://github.com/jaegertracing/jaeger/issues/5270"),
)

@ADI-ROXX
Custom test
10cddb3 
Signed-off-by: cs-308-2023 <[email protected]>
@ADI-ROXX
Lint
4b032d3 
Signed-off-by: cs-308-2023 <[email protected]>
@ADI-ROXX
Copy link
Contributor Author

@yurishkuro I have added a custom test. All the tests are being passed.

yurishkuro
yurishkuro reviewed Jan 26, 2025
View reviewed changes
cmd/jaeger/internal/features/display.go Outdated
if f == nil {
return ""
}
out := ""
Copy link
Member

Choose a reason for hiding this comment

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

why use out += concatenation?

        var buffer bytes.Buffer
        w := io.StringWriter(&buffer)
        // use fmt.Fprintf(w, format, values...)
        return buffer.String()

@ADI-ROXX
Solved issues
b30631c 
Signed-off-by: cs-308-2023 <[email protected]>
yurishkuro
yurishkuro approved these changes Jan 26, 2025
View reviewed changes
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

Thanks!

@yurishkuro yurishkuro changed the title Add 'features' command to beautify and print feature gates Add 'features' command to print available feature gates Jan 26, 2025
@yurishkuro
Copy link
Member

please resolve conflics

@ADI-ROXX
Resolved conflicts
8f2ead7 
Signed-off-by: cs-308-2023 <[email protected]>
@yurishkuro
Copy link
Member

did you not run the test locally?

--- FAIL: TestDisplay (0.00s)
panic: invalid ID "jaeger.features-display.test-gate": invalid character(s) in ID [recovered]
	panic: invalid ID "jaeger.features-display.test-gate": invalid character(s) in ID

@ADI-ROXX
Removed illegal characters
bfc6cac 
Signed-off-by: cs-308-2023 <[email protected]>
@yurishkuro yurishkuro enabled auto-merge (squash) January 26, 2025 17:35
@ADI-ROXX
Copy link
Contributor Author

Working on codecoverage

@ADI-ROXX
Copy link
Contributor Author

How to check code coverage on local?

@yurishkuro
Copy link
Member

go test -cover. I often use vscode package coverage command

@ADI-ROXX
Code coverage
ca4db7e 
Signed-off-by: cs-308-2023 <[email protected]>
auto-merge was automatically disabled January 26, 2025 19:13

Head branch was pushed to by a user without write access

@yurishkuro yurishkuro merged commit 5caf4e4 into jaegertracing:main Jan 26, 2025
55 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yurishkuro yurishkuro yurishkuro approved these changes

@pavolloffay pavolloffay Awaiting requested review from pavolloffay pavolloffay is a code owner automatically assigned from jaegertracing/jaeger-maintainers

Assignees
No one assigned
Labels
changelog:bugfix-or-minor-feature enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add "features" command to print documentation about all features
2 participants
@ADI-ROXX @yurishkuro