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.

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

Fix: service operations fetch by URL query params #932

Merged
merged 3 commits into from
Apr 16, 2022

Conversation

FloydJohn
Copy link
Contributor

Which problem is this PR solving?

Resolves #920

Short description of the changes

If query parameters are provided to the search page, the system now fetches the correct set of operations linked to the specified service.

This fixes the current behavior on fetching the operations linked to the last searched service, ignoring all specified URL parameters.

If query parameters are provided to the search page, the system
now fetches the correct set of operations linked to the specified
service.

This fixes the current behavior on fetching the operations linked to
the last searched service, ignoring all specified URL parameters.

Signed-off-by: Alessandro Racheli <alessandroracheli@gmail.com>
@codecov
Copy link

codecov bot commented Apr 16, 2022

Codecov Report

Merging #932 (1f72e8b) into main (60f1bc8) will decrease coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #932      +/-   ##
==========================================
- Coverage   95.50%   95.43%   -0.07%     
==========================================
  Files         240      240              
  Lines        7518     7520       +2     
  Branches     1886     1887       +1     
==========================================
- Hits         7180     7177       -3     
- Misses        332      337       +5     
  Partials        6        6              
Impacted Files Coverage Δ
.../jaeger-ui/src/components/SearchTracePage/index.js 87.95% <100.00%> (+0.29%) ⬆️
...mponents/TracePage/TraceStatistics/tableValues.tsx 96.89% <0.00%> (-2.42%) ⬇️
...nents/TracePage/TraceTimelineViewer/SpanBarRow.tsx 100.00% <0.00%> (+5.55%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 60f1bc8...1f72e8b. Read the comment docs.

@@ -62,7 +62,7 @@ export class SearchTracePageImpl extends Component {
fetchMultipleTraces(needForDiffs);
}
fetchServices();
const { service } = store.get('lastSearch') || {};
const { service } = urlQueryParams || store.get('lastSearch') || {};
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a JS expert, but this looks odd to me - queryParams is an object, right? So wouldn't it evaluate to true and short circuit the rest even if the objects doesn't have a 'service' property?

I would like to see a unit tests.

Copy link
Contributor Author

@FloydJohn FloydJohn Apr 16, 2022

Choose a reason for hiding this comment

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

I think that urlQueryParams can be undefined (if no query is passed) or an object otherwise. After this comment I played a bit with the UI and it seems that if a param is passed (i.e. end), then the service param is mandatory.

To be absolutely sure of this behavior though I updated the code to explicitly overwrite the service variable only if the service param is provided, in a way that supports both undefined and {} values of urlQueryParams.
I also added and updated unit test to verify that:

  1. If urlQueryParams={}, the system fetches the operations for the stored service
  2. If urlQueryParams includes service, the system works as expected

As I'm quite new to this project and to open source contributions in general, let me know if there's anything I should improve.

Thanks!

This commit ensures that the service to be used for fetching operations
is overwritten only if it's really provided in the URL params.

Signed-off-by: Alessandro Racheli <alessandroracheli@gmail.com>
This commit adds and updates unit tests to ensure that
1. If the "service" parameter is present on the search URL, the
   component fetches the correct operations set
2. If no "service" parameter is present, the component fetches the
   operations linked to the last service searched

Signed-off-by: Alessandro Racheli <alessandroracheli@gmail.com>
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 merged commit 2d91204 into jaegertracing:main Apr 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trace Search: setting operation query param not populating menu item
2 participants