-
Notifications
You must be signed in to change notification settings - Fork 484
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
Search results link spans #536
Search results link spans #536
Conversation
Signed-off-by: Everett Ross <[email protected]>
Signed-off-by: Everett Ross <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #536 +/- ##
==========================================
+ Coverage 92.85% 92.93% +0.07%
==========================================
Files 197 197
Lines 4814 4840 +26
Branches 1162 1174 +12
==========================================
+ Hits 4470 4498 +28
+ Misses 303 302 -1
+ Partials 41 40 -1
Continue to review full report at Codecov.
|
Signed-off-by: Everett Ross <[email protected]>
[trace0]: span0, | ||
}, | ||
}) | ||
).toBe(`/search?span=${span0}%40${trace0}&traceID=${trace2}`); |
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.
Just FYI, in our use case, you won't find a mix of span@trace1&traceId=${trace2}, it's OK for UI can support that.
We should add a test case for:
${span1_1}%20${span1_2}%40${trace1}
&${span2_1}%40${trace2}
&%40${trace3}
Notice trace3 has empty spanIds
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.
Ah, I didn't realize spanIDs were optional.
I added a check & a test for that.
@@ -45,7 +45,7 @@ describe(ReferenceLink, () => { | |||
|
|||
it('render for external trace', () => { | |||
const component = shallow(<ReferenceLink reference={externalRef} focusSpan={focusMock} />); | |||
const link = component.find('a[href="/trace/trace2/uiFind?=span2"]'); | |||
const link = component.find('a[href="/trace/trace2?uiFind=span2"]'); |
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.
+1
import prefixUrl from '../../../utils/prefix-url'; | ||
|
||
import { TNil } from '../../../types'; | ||
|
||
export const ROUTE_PATH = prefixUrl('/trace/:id'); | ||
|
||
export function getUrl(id: string) { | ||
return prefixUrl(`/trace/${id}`); | ||
export function getUrl(id: string, uiFind?: string): string { |
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.
Not very familiar with tsx syntax, is "uiFind?" expected with "?"
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.
the ?
just means it's optional. not all trace urls are deep links.
Signed-off-by: Everett Ross <[email protected]>
).toBe(`/search?traceID=${trace0}`); | ||
}); | ||
|
||
it('converts spanLink and traceID to traceID and span', () => { |
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.
to traceID and span
or just to span
?
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.
good catch, fixed here:
https://github.com/jaegertracing/jaeger-ui/pull/539/files
* Add span linking to search results * Add more url tests * Change span csv to space seperated value * Handle span param without spanIDs Signed-off-by: Everett Ross <[email protected]> Signed-off-by: vvvprabhakar <[email protected]>
Which problem is this PR solving?
getExternalLink
forReferenceLink
used when a span has external referencesShort description of the changes
SearchTracePage/url
parsesspan
intospanLinks
andtraceID
(unchanged:traceID
is used to fetch traces instead of searching)spanLinks
are given toSearchResults
to pass auiFind
value to theTracePage
(unchanged: this is how deep linking intoTracePage
already behaves)