Add JSON button to trace page #1124
Conversation
| @@ -0,0 +1,16 @@ | |||
| import {component} from 'flightjs'; | |||
| import $ from 'jquery'; | |||
There was a problem hiding this comment.
Defined, but never used (try running npm run lint)
|
I had some comments; otherwise, great work! |
|
Thanks for the effort and review folks. This is very popular!
|
|
Thanks for the comments, I'll address these tonight or tomorrow. |
|
Comments addressed. |
|
running it locally and will merge (and release) after! |
|
I've got another set of changes coming too dealing with better error messages, just testing it now. |
|
ah ok thx for the heads-up. this is a different PR, right? |
|
Yep. Different PR. |
| const self = this; | ||
| self.raw = data.raw; |
There was a problem hiding this comment.
how is this controlled?
There was a problem hiding this comment.
It comes through the trigger in js/component_data/trace.js
There was a problem hiding this comment.
There was a problem hiding this comment.
This PR does not provide a mechanism to do that. I suspect that is best saved for future work -- if there isn't a way to show an uploaded trace, no sense downloading the trace?
I could be convinced otherwise, it would be easy to add an actual raw download link somewhere.
There was a problem hiding this comment.
There was a problem hiding this comment.
Oh, ha. Didn't realize that.
There was a problem hiding this comment.
ex not sure why the website doesn't render it, but here's the definition
- name: raw
in: query
required: false
description: |
Note this flag has no value. Ex. /trace/{traceId}?raw
Normally, the trace endpoint cleans trace data. For example, it merges
spans by id, adds missing timestamp or duration, corrects clock skew..
Specifying this flag is a debug case, when you are debugging zipkin
logic or zipkin instrumentation, and want to see the input to these
adjusters. For example, this might explain or rule out clock skew.
type: boolean
https://github.com/openzipkin/zipkin-api/blob/master/zipkin-api.yaml#L146
There was a problem hiding this comment.
Interesting. I suppose we could add a mechanism that could display either. I chose the default since it was already retrieved by the server -- so it would show the JSON that's currently being displayed without another call to the server. Theoretically, if you made another call down, the JSON could show something different which wasn't currently being displayed.
There was a problem hiding this comment.
There was a problem hiding this comment.
Renamed to traceJson
|
fyi here are some screen shots:
|
|
is json raw or merged? |
|
The JSON should be whatever was retrieved from the call to |
|
@yurishkuro you have this feature already.. do you default to raw json? any thoughts about what the default should be? |
|
@adriancole we added two buttons, raw and regular json, as both come in handy. But we just have them as plain links to json documents, not popups, and we leave pretty rendering to browser plugin. That way it's easy to search inside the json, and do a Save As. |
|
interesting. search works fine with the pop-up, but save-as doesn't. It is |
|
Here's a thought. you can add a save or download link to the pop-up. this could point to /api/v1/trace/id for example. |
|
+1 for adriancole's suggestion! |
|
I added a save icon, and also the ability to right click on the JSON button and do a save as (as that's an intuitive option anyways). |
| @@ -167,6 +168,7 @@ export default function traceToMustache(trace) { | |||
| const spansBackup = spans; | |||
|
|
|||
| return { | |||
| traceId, | |||
There was a problem hiding this comment.
I guess this isn't needed anymore?
There was a problem hiding this comment.
It's used in trace.mustache line 7.
- Shows prettified JSON in modal dialog - Adds download link for JSON - Fixes #1060
| ServiceFilterSearchUI.attachTo('#serviceFilterSearch'); | ||
| SpanPanelUI.attachTo('#spanPanel'); | ||
| TraceUI.attachTo('#trace-container'); | ||
| FilterLabelUI.attachTo('.service-filter-label'); | ||
| ZoomOut.attachTo('#zoomOutSpans'); | ||
|
|
||
| this.$node.find('#traceJsonLink').click(e => { |
There was a problem hiding this comment.
@eirslett you're right, this ends up being cleaner anyways, it never really belonged in trace.js
|
Otherwise, LGTM! ship it |
|
yay json button! |
Well, since the "privileged minority" hasn't done it yet, I figured I would since I keep finding that I need it. :)
I'm very new to the various frameworks that the UI is using, so I'm open to suggestions if there's anything that I did that was a bit wonky.