Add Import json from file

[yuribit]

Which problem is this PR solving?

• Resolves "Import JSON file" option in Jaeger UI #214

Short description of the changes

• The search trace page has two tabs, the first one contains the old form while the second tab contains a simple file uploader (see screenshot below).

[codecov]

Codecov Report

Merging #327 into master will increase coverage by 0.02%.
The diff coverage is 85%.

@@            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

Impacted Files	Coverage Δ
...er-ui/src/components/SearchTracePage/FileLoader.js	100% <100%> (ø)
packages/jaeger-ui/src/reducers/trace.js	98.59% <100%> (+0.37%)	⬆️
packages/jaeger-ui/src/actions/file-reader-api.js	100% <100%> (ø)
.../jaeger-ui/src/components/SearchTracePage/index.js	88.13% <50%> (-1.34%)	⬇️
packages/jaeger-ui/src/utils/fileReader.js	72.22% <72.22%> (ø)

---

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 ca0454c...1fdc02c. Read the comment docs.

[yurishkuro]

Thanks for this is awesome feature!

I have a few questions on functionality:

1. Why did you decide to add it to Search, rather than adding a top-level menu?
2. What happens after you drag a file with one trace, does it show the trace view immediately or shows the trace in the list of "search results"?
3. Once you open the trace view, what does the URL look like?
4. I assume multiple traces must be in a JSON array, maybe worth mentioning it. We have a way of downloading a JSON for one trace via the dropdown in the trace view, but there's no way of downloading >1 traces in this array format (maybe we need a DL button on the search results?)
5. Can you drag more than one file instead of a file with a JSON array? That might be more usable UI.

[yuribit]

@yurishkuro thanks for your comments, please see my answers below:

1. Why did you decide to add it to Search, rather than adding a top-level menu?

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.
Another idea was to add a Dropdown Item labelled "Upload trace" in the "About Jaeger" dropdown in the top-level menu.
I am open to suggestions, feel free to tell me which option you like better and I'll make the necessary adjustments.

2. What happens after you drag a file with one trace, does it show the trace view immediately or shows the trace in the list of "search results"?

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.

3. Once you open the trace view, what does the URL look like?

Very good point. The url looks like this one: /trace/36afc9f9fa8a0db8. If you refresh the page, the app will not be able to reload the trace. Do you think I should add a parameter to know when the trace is loaded from file? E.g. /trace/36afc9f9fa8a0db8?loadedFromFile=True

4. I assume multiple traces must be in a JSON array, maybe worth mentioning it. We have a way of downloading a JSON for one trace via the dropdown in the trace view, but there's no way of downloading >1 traces in this array format (maybe we need a DL button on the search results?)

I can modify the help in the upload panel and add a download button in the Search Result Panel.

5. Can you drag more than one file instead of a file with a JSON array? That might be more usable UI.

I agree. I can add the multi upload functionality so we can support both scenarios.

[yurishkuro]

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. /uploaded-trace/{traceid}, so that Refresh works as expected. Perhaps we could also store the uploaded traces in the local storage, this way even Refresh will work with uploaded traces until that trace expires.

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.

[yuribit]

@yurishkuro, I added support for multiple file upload. Here is a small preview:

[yurishkuro]

🎉 🎈 🎉 🎈 🎉 🎈 🎉 🎈

[tiffon]

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.

[yuribit]

@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.
If we don't, then we could leave the URL as is and manage everything in the Redux state either with a dedicated section or using a flag. I would also prefer to manage the active tab in Redux. In this way that info will be cleared when the user refreshes the page.

[tiffon]

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?

[yuribit]

@tiffon, I'll apply the changes asap. Should I also move the "loaded" trace in its own redux section state.trace.local?
Is it ok if I upgrade the version of antd from 3.8.0 to 3.13.6? I need to customize the tab bar to properly fix the style. That feature was introduced with version 3.9.0 and bugfixed on the following versions.

[tiffon]

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?

[yuribit]

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.

[tiffon]

Sorry, can you change the title of the tab to use "find" instead of "search"?

@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 fileReader.readJsonFile; it was possible to have an unhandled error in the promise constructor callback. I also hit a few more LOCs in the fileReader tests.

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!

[yuribit]

Nice!! Thank you again for the review and fixing the error.

[tiffon]

@yuribit Thanks for putting together this fantastic feature! 🎉

[yurishkuro]

Great work, team!

@pavolloffay wants to do a new release next week, let's make sure we have a UI release with this included.

[omriperi]

Hey all! The only thing I don't understand from this feature, from where can I get this JSON?
If I'm having a collector in my system, can I query it for some JSON file? Does he stores JSON file inside him? Does it generates a JSON file?
What's the difference between unparsed to parsed JSON file and where each one of them resides?
Thanks!

[yurishkuro]

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.

[omriperi]

@yurishkuro - Thanks
You are talking about this API, right?
https://www.jaegertracing.io/docs/1.16/apis/#http-json-internal

[yurishkuro]

yes

[omriperi]

thanks a lot :)

[piano-man]

Hi folks!
Just wanted to understand the scope of this feature a little better.

1. Why do we not store the uploaded traces in local storage such that the traces persist on reload?
2. Are there any plans to support tag-based filtering on these uploaded traces just like we do for the search option?