Adding the ability to import traces from JSON into zipkin-ui #1884
Conversation
|
interesting! for a impl summary, is the trace in browser local storage or is it sortof a one-shot deal? Can you attach screen shots? Is there anything sharable between traceViewer.mustache and anything else? |
Edit: updated images to reflect new link name. |
|
kindof chummy but I almost like "paste a trace" better. Import might
confuse people, or at least it did me.
|
|
You da boss! Say the words and I will make the change :) |
|
some dangling concerns are that the UI doesn't natively speak v2 yet, so however worded, this implies the data is in v1 format. The v2 format is in zipkin-js.. I wonder if there's a way we can leverage that to allow pasting of v2. If we have this feature, it should work with new data. PS still need others feedback cc @openzipkin/core |
|
So, I just realized I messed up the commit a bit (new to and hating git, sorry). So bear with me for a bit while I try to correct things. Edit: Corrected but in 2 commits this time :\ Also, I do agree this should work with v2 data. But at the moment I'm not sure where zipkin-js is or how to update it to work with v2 data. I'd be happy to help with what I can given the right direction but for now I'll wait for the feedback from others. |
| reader.onload = evt => { | ||
| let model; | ||
| try { | ||
| const trace = JSON.parse(evt.target.result); |
There was a problem hiding this comment.
I think here, we should assume zipkin v2 format unless we know heuristically it is not. Worst case we can copy/paste a modified version of https://github.com/openzipkin/zipkin-js/blob/master/packages/zipkin/src/jsonEncoder.js#L25 to map v2 json to v1 format. Ex here are the tests for that: https://github.com/openzipkin/zipkin-js/blob/master/packages/zipkin/test/jsonEncoder.JSON_V1.test.js
Reason is that it is only time resource which is why the UI isn't internally rewritten to v2 yet. I don't want to encourage people to keep using v1. The best heuristic to detect if incoming json is v2 or not is presence of the "localEndpoint" field, which is only absent on instrumentation error.
There was a problem hiding this comment.
Are you suggesting that we iterate through the spans in trace, see if they have a 'localEndpoint', and if they do, then call jsonEncoder.encodeV1() (effectively) on each span so that the UI can display both v1 and v2 traces?
I'm midly afraid that v2 traces with 10k spans will suffer a bit from that. But that concern aside, is there an easy way to extract jsonEncoder without having to copy/paste it into zipkin-ui? Of course, I'm talking about extracting/sharing the conversion more so than the call to JSON.stringify() at the end.
I think here, we should assume zipkin v2 format unless we know heuristically it is not.
Given that the UI will only show you traces in v1 format I think it is safer to assume v1 until zipkin-ui is updated to support v2. But that doesn't mean we can't still support v2 pending the above discussion.
|
Haha if you have a trace with 9k spans you will have other problems.
In any case better to profile the concern and check what the impact is.
At some point the native format will be v2 which means this conversion will
not happen so it is temporary.
…On 20 Jan 2018 6:55 am, "Logic-32" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In zipkin-ui/js/component_ui/uploadTrace.js
<#1884 (comment)>:
> +import {component} from 'flightjs';
+import FullPageSpinnerUI from '../component_ui/fullPageSpinner';
+import traceToMustache from '../../js/component_ui/traceToMustache';
+
+export default component(function uploadTrace() {
+ this.doUpload = function() {
+ const files = this.node.files;
+ if (files.length === 0) {
+ return;
+ }
+
+ const reader = new FileReader();
+ reader.onload = evt => {
+ let model;
+ try {
+ const trace = JSON.parse(evt.target.result);
Are you suggesting that we iterate through the spans in trace, see if
they have a 'localEndpoint', and if they do, then call
jsonEncoder.encodeV1() (effectively) on each span so that the UI can
display both v1 and v2 traces?
I'm midly afraid that v2 traces with 9k spans will suffer a bit from that.
But that concern aside, is there an easy way to extract jsonEncoder without
having to copy/paste it into zipkin-ui? Of course, I'm talking about
extracting/sharing the conversion more so than the call to
JSON.stringify() at the end.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1884 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAD611XWjJmtPktO-g2zMz4u_9ItgiPsks5tMR11gaJpZM4RfE7z>
.
|
|
Looping through 10k elements in JS is still quicker than touching the DOM, I don't think it will be a problem. (But don't take my word for it, might be a good idea to profile it.) I would be concerned if it took more than 30ms. |
|
Could copy/paste with a comment or add a dependency on zipkin-js and use
the functions directly. Usually we do rule of three on extracting code in
upstream projects (ex hopefully arent many use cases for converting to v1
so ideally we dont end up with 3 requests!) . Maybe see how it goes first
then we can handle that as it arises (easier to see impact this way?
Appreciate your effort and patience!
On 20 Jan 2018 7:43 am, "Adrian Cole" <[email protected]> wrote:
Haha if you have a trace with 9k spans you will have other problems.
In any case better to profile the concern and check what the impact is.
At some point the native format will be v2 which means this conversion will
not happen so it is temporary.
…On 20 Jan 2018 6:55 am, "Logic-32" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In zipkin-ui/js/component_ui/uploadTrace.js
<#1884 (comment)>:
> +import {component} from 'flightjs';
+import FullPageSpinnerUI from '../component_ui/fullPageSpinner';
+import traceToMustache from '../../js/component_ui/traceToMustache';
+
+export default component(function uploadTrace() {
+ this.doUpload = function() {
+ const files = this.node.files;
+ if (files.length === 0) {
+ return;
+ }
+
+ const reader = new FileReader();
+ reader.onload = evt => {
+ let model;
+ try {
+ const trace = JSON.parse(evt.target.result);
Are you suggesting that we iterate through the spans in trace, see if
they have a 'localEndpoint', and if they do, then call
jsonEncoder.encodeV1() (effectively) on each span so that the UI can
display both v1 and v2 traces?
I'm midly afraid that v2 traces with 9k spans will suffer a bit from that.
But that concern aside, is there an easy way to extract jsonEncoder without
having to copy/paste it into zipkin-ui? Of course, I'm talking about
extracting/sharing the conversion more so than the call to
JSON.stringify() at the end.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1884 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAD611XWjJmtPktO-g2zMz4u_9ItgiPsks5tMR11gaJpZM4RfE7z>
.
|
Lol. Perhaps. But it is an unfortunate fact of life in the system I work on. It's not really all that common but I deal with the worst-of-the-worst so I have the unfortunate pleasure of seeing it daily :'(
Touche! My head must be in the clouds right now :)
Will start with copy/paste and update the pull request when I can. No promise on ETA given work priorities and such :\ |
|
Back to working on this.
Just found out that with partials I can.
Also, I renamed "Import A Trace" to "View Saved Trace". I added some missing i18n properties as well. No translations, just the properties and default values. Hopefully that doesn't mess with the foreign languages too much :\ Reference pictures in prior post updated to reflect these changes. (commit to follow...) |
| nav.search = Go to trace | ||
| nav.viewSaved = View Saved Trace |
There was a problem hiding this comment.
@MrGlaucus @gianarb @drolando can you contribute chinese and italian text for this?
There was a problem hiding this comment.
I will work on the translation as the merge is done.
|
Isn't the JSON returned by the UI valid V2 format? Couldn't you simply POST it back to /api/v2/spans and use the UI as usual? |
|
@drolando right now the UI doesn't mount any write endpoints, and search etc is limited by original timestamp. to change to mount write endpoints might interfere with some folks assumptions about the security of their UI (right now, if you cut at /zipkin/* there's no way to poison or otherwise trace data). There is a different change that wasn't universally popular about writing to a separate storage service #1747. On the technical bits, the v2 conversion etc code will end up replacing the v1 code in the project, so some of this is temporary technical debt. hope this background is useful |
|
@drolando, in addition to what Adrian said, where I work we have a habit of attaching trace JSON to bug cases as output via the JSON button in the UI. Without this feature, or some other tool to import the JSON, the trace JSON is pretty useless. We have no desire to actually re-persist the JSON since our ES cluster is already pretty full of Zipkin data. So this makes for a nice on-the-fly viewing of data with no storage overhead. @adriancole, any updates on feedback from the core team? |
|
PS I am also in favor of this feature as I routinely need to check people's
traces and right now I have to post in order to do so. For support, this is
a nice thing, as not always is the problem I'm looking at storage specific
(usually it is instrumentation specific)
|
zipkin-ui/js/main.js
Outdated
| @@ -21,6 +22,7 @@ loadConfig().then(config => { | |||
|
|
|||
| crossroads.addRoute(contextRoot, () => initializeDefault(config)); | |||
| crossroads.addRoute(`${contextRoot}traces/{id}`, traceId => initializeTrace(traceId, config)); | |||
| crossroads.addRoute('zipkin/traceViewer', () => initializeTraceViewer(config)); | |||
There was a problem hiding this comment.
shouldn't this be using the contextRoot variable? (if not, those proxy-mounting zipkin will be broken I think.. look at the README for more)
There was a problem hiding this comment.
Woops! My bad. Will fix.
|
@Logic-32 can you look at the test dir and see if you can make a similar test for anything that's new here (possibly for the conversion part, copy some test code from zipkin-js). This is a lot of new code and having at least some coverage will help if there's a bug we need to sort out later |
|
PS the UI still says "view a saved trace" which is confusing as data in zipkin is saved. I wonder if the upload functionality should be a widget next to "go to trace" Ex instead of going to a trace by ID, we are going to an offline trace (by choosing the file). Regardless, I feel a bit off about the json viewer being mixed in with server side stuff like dependencies, as it is offline.. One way is to change "go to trace" to "enter trace ID" and put another widget directly under that says "choose trace json" I tried the functionality and notice I can't collapse any of the spans (even clicking expand-all collapse-all). Might want to check that. |
That's because I changed it from "Import a Trace" ;)
Maybe? It could certainly clear up the verbiage issue we're having. I attached a rough mockup below of what it would look like (in IE). Having it below the "go to trace" box isn't really practical as I don't think Bootstrap navs support multi-row very well (at least, I've never seen one). Either way, I can't say it looks very clean. It's not immediately obvious which textbox should be used for what purpose without some descriptive text around the file input. Additionally, there are some workflow complications with doing it that way. After selecting the file we'd have to change the URL to remove any reference to current state. Otherwise somebody may try to copy/paste it and expect the same results. Which won't happen. We can use JS to fix the URL but we have to be careful with history management so the back/forward buttons work as expected. So the short version is that I believe a dedicated page for viewing the traces is the simplest way to do it. But I'm not sure how to disambiguate the online from offline stuff like you're concerned about. We can consider a multi-level nav but that means extra clicks to get to a page. With as simple as the UI is at the moment I don't think that's appropriate either.
Works on my box in Zipkin 2.4.6 with this change applied. Tried IE, FF, and Chrome. Behaves identically to normal/stored traces. Did those work for you? Which version/branch were you using? p.s. Will try to find a test to add when I can. |
|
Just got tests added for the span conversion stuff. Unfortunately there is a somewhat higher level bug that may make them moot. Currently, the /zipkin/v1/trace/{traceIdHex} has an optional query parameter to dump raw traces. From what I can tell, getting that dump avoids some timestamp adjustments and sorting that a non-raw dump would do. Unfortunately, it seems like /zipkin/v2/trace/{traceIdHex} defaults to the same /v1 raw behavior. Now, it's unfortunate because the UI doesn't like it when the initial/root span isn't the first in the list of spans. It actually fails quite miserably when that's the case. Additionally, the lack of timestamp adjustments can cause spans to render in odd places on the timeline. Is this something that would hold up this feature? If so, I have no idea how I'd handle the situation at the moment. For reference:
p.s. How does "View Local Trace" sound? |
|
Thanks for the update and all the hard work @Logic-32. yes, we decided that in v2 it would be best to move the adjustment logic to javascript in the UI (which makes a fake root span etc), just this hasn't been done. I forgot about this. The key part is this logic (around sorting the trace by the root span) https://github.com/openzipkin/zipkin/blob/master/zipkin/src/main/java/zipkin/internal/ApplyTimestampAndDuration.java There is also clock skew logic, which would might be something we can punt slightly. |
|
Happy to help @adriancole :) However, I'm not sure I'm up for porting those changes over. I know Java/JS well enough but it's a lot of testing/etc... and some other priorities have come up for me. Is it a requirement to have those working before the change can be merged? If so, what other options do we have? At a minimum, I could probably implement a sort routine to at least make sure the root span is first in the list. That's about all I'd have time for. And if I did that, would you want me to do it for raw v1 traces as well? |
|
@Logic-32 thanks for being transparent about what you can budget attention for. It has been a great help so far. I think that the sort-based thing should happen after any potential conversion to v1 and before the existing logic is invoked. What I mean is that I'd solve order for v1 raw (which should transitively solve for v2). Limit to sorting and then we'll open an issue to progress further. Sound good? |
|
Sounds good to me! Will update when I can. |
|
another release will be on the way soon. any chance this might be ready? |
|
Was able to look at sorting a little bit last week. Going to try and polish/commit today :) Edit: done. |
… navbar to accomodate new link. Adding configuration to forward requests to traceViewer Updated traceViewer to support v2 traces by recoding in v1 format. Add error handling in case JSON parsing fails. Renamed feature to 'View Saved Trace'. Added il8n properties. Extracted span conversion into it's own file so it could be tested more easily. Fixed missing 'contextPath' issue.
… somewhat correctly.
|
testing now |
|
I will test this with normal span. then merge and add code for messaging span. Good stuff! |
|
there's some interesting offset rendering I'm looking into.. |
|
for example, the UI is misaligned unless you stretch the screen carefully
|
|
Looks like the left positioning is present on trace viewer but not the normal trace screen |
|
getting somewhere now.. |
|
ok it is because when we switch to v1 format we aren't merging by ID like the v1 api does.
|
|
ok since this is on your master branch, I'm not going to push a commit to it. I'll make a commit to fix the merging thing and squash yours into it on the way in. will finish up tonight. Thanks for the help! |
|
To make sure I understand correctly:
|
This is an alternative to saving on the backend or via a proxy Closes #1747
|
Awesome! Thank you for fixing up that merging issue for me :) |
This is an alternative to saving on the backend or via a proxy Closes openzipkin#1747
I got tired of having no way to view previous traces in Zipkin. Since it provides a way to download the trace JSON I figured there should be a way to add it back. This adds a new link with the ability to select a JSON file for upload (as opposed to copy/paste due to the potential size of traces, ref: #1460) and view it just like any other trace.
Relates to: #1747.