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

Make ArchiveTrace button auto-configurable #4913

Merged
merged 5 commits into from
Nov 7, 2023
Merged

Make ArchiveTrace button auto-configurable #4913

merged 5 commits into from
Nov 7, 2023

Conversation

thecoons
Copy link
Contributor

@thecoons thecoons commented Oct 31, 2023 •
edited by yurishkuro
Loading

Which problem is this PR solving?

Description of the changes

The button to archive a trace is now configured based on the state of
the QueryService in addition to the UI configuration. It is now
possible to request features from the QueryService to inject them
into the UI.

Related UI change jaegertracing/jaeger-ui#1944

How was this change tested?

All corresponding tests have been updated.

Checklist

Signed-off-by: Antonin Barthelemy antobarth@gmail.com

@thecoons thecoons requested a review from a team as a code owner October 31, 2023 16:29
@thecoons thecoons marked this pull request as draft October 31, 2023 16:30
@thecoons
Copy link
Contributor Author

I am still working on the feature but would appreciate any feedback on the design choices. Thank you in advance.

yurishkuro
yurishkuro reviewed Oct 31, 2023
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.

as I can see you're not actually using the capability flag in generating the UI config.

I suggest following the pattern already used in index.html and defining another function getStorageCapabilities to be substituted by the query-service's static handler. I just added a bit of documentation how this is currently done: jaegertracing/jaeger-ui#1926

cmd/query/app/querysvc/query_service.go Outdated Show resolved Hide resolved
cmd/query/app/querysvc/query_service.go Outdated Show resolved Hide resolved
cmd/query/app/querysvc/query_service.go Outdated Show resolved Hide resolved
cmd/query/app/static_handler.go Outdated Show resolved Hide resolved
@thecoons
Copy link
Contributor Author

thecoons commented Nov 1, 2023

as I can see you're not actually using the capability flag in generating the UI config.

I suggest following the pattern already used in index.html and defining another function getStorageCapabilities to be substituted by the query-service's static handler. I just added a bit of documentation how this is currently done: jaegertracing/jaeger-ui#1926

I've taken a look. Do you think we should include the QueryServiceCapabilities within the redux.config state, or should we create a dedicated state?

@yurishkuro
Copy link
Member

Yes, we can add it to redux.config

@thecoons thecoons force-pushed the archive_button_auto_config branch 2 times, most recently from 4dcfc50 to 6e441f0 Compare November 3, 2023 13:29
yurishkuro
yurishkuro reviewed Nov 4, 2023
View reviewed changes
cmd/query/app/static_handler.go Outdated Show resolved Hide resolved
cmd/query/app/static_handler.go Outdated Show resolved Hide resolved
@thecoons
Make ArchiveTrace button auto-configurable
74f60a4 
- Resolves jaegertracing#4874

The button to archive a trace is now configured based on the state of
the QueryService in addition to the UI configuration. It is now
possible to request features from the QueryService to inject them
into the UI.

All corresponding tests have been updated.

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

---------

Signed-off-by: Barthelemy Antonin <antobarth@gmail.com>
@thecoons
Copy link
Contributor Author

thecoons commented Nov 7, 2023

I always have problems integrating the feature, can you confirm that in its current state the feature is not functional?

@yurishkuro
Use handler as receiver, not options
36f27b1 
Signed-off-by: Yuri Shkuro <github@ysh.us>
@codecov Codecov
Copy link

codecov bot commented Nov 7, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Files Coverage Δ
cmd/query/app/querysvc/query_service.go 100.00% <100.00%> (ø)
cmd/query/app/server.go 94.57% <100.00%> (ø)
cmd/query/app/static_handler.go 97.85% <100.00%> (+0.07%) ⬆️

📢 Thoughts on this report? Let us know!

@yurishkuro
Undo unnecessary changes
d22ba2d 
Signed-off-by: Yuri Shkuro <github@ysh.us>
@yurishkuro
Undo unnecessary changes
09d117b 
Signed-off-by: Yuri Shkuro <github@ysh.us>
@yurishkuro yurishkuro added the changelog:new-feature Change that should be called out as new feature in CHANGELOG label Nov 7, 2023
@yurishkuro yurishkuro marked this pull request as ready for review November 7, 2023 23:07
yurishkuro
yurishkuro approved these changes Nov 7, 2023
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.

lgtm

@yurishkuro yurishkuro merged commit c17b575 into jaegertracing:main Nov 7, 2023
35 checks passed
@thecoons
Copy link
Contributor Author

thecoons commented Nov 8, 2023

Thank you for the help; I'll try to be more independent with the upcoming features on Jaeger.

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:new-feature Change that should be called out as new feature in CHANGELOG
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@thecoons @yurishkuro