-
Notifications
You must be signed in to change notification settings - Fork 497
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
Add Import json from file #327
Conversation
Codecov Report
@@ Coverage Diff @@
## master #327 +/- ##
==========================================
+ Coverage 83.08% 83.11% +0.02%
==========================================
Files 142 145 +3
Lines 3181 3221 +40
Branches 654 656 +2
==========================================
+ Hits 2643 2677 +34
- Misses 431 436 +5
- Partials 107 108 +1
Continue to review full report at Codecov.
|
Thanks for this is awesome feature! I have a few questions on functionality:
|
@yurishkuro thanks for your comments, please see my answers below:
In my mind, the Upload page and the Search page would have looked very similar - panel on the left and results on the right. They also share the same behaviors so I decided to add a new panel within the Search page.
At the moment it simply shows the trace in the list of Search Results, similar to what the Search Form does. I like it because the Search Result Item shows a recap of the trace with very useful info. I can change it and show the trace view immediately but then I think we should also change the Search Form to behave in the same way.
Very good point. The url looks like this one:
I can modify the help in the upload panel and add a download button in the Search Result Panel.
I agree. I can add the multi upload functionality so we can support both scenarios. |
1 - I am fine with keeping it the way you have it, at least in the first version until we get more feedback. 2 - showing trace in the search results is better, because it would work with being able to upload more than one trace (e.g. if I want to diff them). 3 - @tiffon and @everett980 should opine on that. I think it's better to have a dedicated URL, e.g. 4 - might want to hold off on that. I think if the panel allows dragging more than one file (including at the same time), then this whole JSON array file support may not be needed. |
6e845f0
to
b3eba6f
Compare
@yurishkuro, I added support for multiple file upload. Here is a small preview: |
🎉 🎈 🎉 🎈 🎉 🎈 🎉 🎈 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting this together! Awesome work!
When returning to the search page after viewing a trace from a JSON file, seems like the search page should remain on the local file tab? We migth want to use the URL route to capture this state.
I'm wondering if we should separate the search state from the state associated with JSON files. This would facilitate comparing local files with search results. Currently, search results are wiped when a new search is executed, which means traces added from local files are also cleared. So, after every search the file(s) will need to be uploaded, again. But, if we add a state.trace.local
section (or something with a better name) to the redux state, then we can persist traces from files in the result list while clearing the search result items. What do you think?
Also, if we differentiate traces from files from traces that were fetched by adding a flag to the state.trace.traces[id]
object, we should be able to have distinct routes for them, more easily.
Awesome work! Let me know what you think of this feedback. I've been a bit vague, maybe we can hash out the details if you have the bandwidth to make these changes.
packages/jaeger-ui/src/components/SearchTracePage/FileUploader.js
Outdated
Show resolved
Hide resolved
packages/jaeger-ui/src/components/SearchTracePage/FileUploader.js
Outdated
Show resolved
Hide resolved
packages/jaeger-ui/src/components/SearchTracePage/FileUploader.test.js
Outdated
Show resolved
Hide resolved
@tiffon, thank you for the review! I applied all the changes you requested but one, please see my comment. I also still need to figure out what's going on with the style in the tab header, might need a custom one. Regarding the changes to the URL, I think it all depends on whether we add the traces to the local storage or not. If we do so, than we need to create a new URL and figure out how to remove them from the search results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the changes! A couple of other things came to mind, namely changing from "upload" to "load" in the various identifiers.
What do you think?
packages/jaeger-ui/src/components/SearchTracePage/FileUploader.js
Outdated
Show resolved
Hide resolved
@tiffon, I'll apply the changes asap. Should I also move the "loaded" trace in its own redux section |
Great to hear! I would say lets keep, as is, with regard to redux; we can evolve the feature after merging the current version. Re upgrading antd, it's currently pinned at the version just before animation for the tag (or is it pill?) component was added. The animation resulted in pretty brutally degraded performance when rendering search results, which use the component to show the number of spans for each service for each trace. I ran into that awhile, so I'm not sure where the component currently stands. If there are perf issues, then we can roll our own component that fits our needs and doesn't have any animation. Or, maybe the animation can be disabled? What is the issue with the tabs that requires the upgrade? |
The issue is the one you pointed out here. When the page is narrow, the tabs become scrollable and there's no way to disable it unless we implement a custom tab bar. |
Signed-off-by: Yuri Roncella <[email protected]>
Bolster the fileReader.readJsonFile tests a bit. Signed-off-by: Joe Farro <[email protected]>
@yuribit Sorry, I got my wires crossed and was totally backwards in that comment. "Search" is the right word; I've fixed it. I also added a bit more error handling to Regarding the tabs, I changed the text to "Search" and "JSON File", which is pretty narrow, so I think it should be ok without a custom tab bar. What do you think? LMK if anything looks awry! |
Nice!! Thank you again for the review and fixing the error. |
Signed-off-by: Joe Farro <[email protected]>
@yuribit Thanks for putting together this fantastic feature! 🎉 |
Great work, team! @pavolloffay wants to do a new release next week, let's make sure we have a UI release with this included. |
Hey all! The only thing I don't understand from this feature, from where can I get this JSON? |
the trace view has a button to download the JSON. You can then use that JSON to upload it again. You can also load the JSON directly from the API in the query service. |
@yurishkuro - Thanks |
yes |
thanks a lot :) |
* Add file uploader Signed-off-by: Yuri Roncella <[email protected]> * s/Find/Search/, handle more errors in readJsonFile Bolster the fileReader.readJsonFile tests a bit. Signed-off-by: Joe Farro <[email protected]> * Update the changelog Signed-off-by: Joe Farro <[email protected]> Signed-off-by: vvvprabhakar <[email protected]>
Hi folks!
|
Which problem is this PR solving?
Short description of the changes