-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Support upload trace in Zipkin Lens UI #2447
Support upload trace in Zipkin Lens UI #2447
Conversation
Oh, I forgot to remove traceId component from search bar... |
Removed TraceId component from search bar... |
thanks! please add the "how it works" image to the README at the bottom under a markdown section named design or a separate DESIGN.md |
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.
Looks good. I like the design and I feel this maximizes the UI functionality without clutter.
one small note about error handling.
pathname: '/zipkin/traceViewer', | ||
}); | ||
} catch (error) { | ||
// Do nothing |
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.
maybe alert the json is malformed? I assume this catch will result in someone uploading an incorrect file (ex just one span) and getting no feedback about it. correct?
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.
Yes.
Do you think that it is better to display the alert dialog?
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.
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.
OK, Thank you.
I'll do so
note: the view trace screen blows up (white screen) if using v1 json
In the old uploadTrace.js we had something like this. If you import it, please also import the unit tests. export function ensureV2(trace) {
if (!Array.isArray(trace) || trace.length === 0) {
throw new Error('input is not a list');
}
const first = trace[0];
if (!first.traceId || !first.id) {
throw new Error('List<Span> implies at least traceId and id fields');
}
if (first.binaryAnnotations || (!first.localEndpoint && !first.remoteEndpoint && !first.tags)) {
throw new Error(
'v1 format is not supported. For help, contact https://gitter.im/openzipkin/zipkin');
}
} |
Looks good besides the blowup. I tested uploading a v2 json and also get trace by ID |
I see! |
goodie! |
Great work, @tacigar! |
Related: #2350
History: #2424