-
Notifications
You must be signed in to change notification settings - Fork 513
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
added function to show full traceID in #2536
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Avinash <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2536 +/- ##
=======================================
Coverage 96.61% 96.61%
=======================================
Files 255 256 +1
Lines 7745 7750 +5
Branches 1935 2003 +68
=======================================
+ Hits 7483 7488 +5
Misses 262 262 ☔ View full report in Codecov by Sentry. |
looks great ... but IMHO it takes extra space as it's showing the User same information as it's already available on top. what you can do is that make the shorten traceID clickable and then clicking on it would expand to show Full ID and again clicking on it would collapse it. This would also clean-up the UI and IMO would be a better UX design |
Thanks! I'll make TraceID clickable for better UX |
Signed-off-by: Avinash <[email protected]>
Hey @MAX-786 |
@avinpy-255 Hi, |
The original issue was asking for configuration to make the trace ID displayed in full. There is no configuration added in this PR. The configuration can be specifying the desired length from the ID to display, e.g. 32 would display full ID. |
Signed-off-by: Avinash <[email protected]>
@@ -20,6 +20,8 @@ import getVersion from '../utils/version/get-version'; | |||
import { version } from '../../package.json'; | |||
import { Config } from '../types/config'; | |||
|
|||
export const TraceIDLength = 7; |
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.
we don't use constants for anything else, just use the value directly
@@ -149,7 +150,7 @@ export function TracePageHeaderFn(props: TracePageHeaderEmbedProps & { forwarded | |||
return { ...rest, value: renderer(trace) }; | |||
}); | |||
|
|||
const traceShortID = trace.traceID.slice(0, 7); | |||
const traceShortID = trace.traceID.slice(0, TraceIDLength); |
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.
add a test?
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, I will add a test for tracIDlength
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.
@@ -135,6 +135,9 @@ export type Config = { | |||
// TODO when is it useful? | |||
scripts?: readonly TScript[]; | |||
|
|||
// traceIDLength controls the length of the trace ID displayed in the UI. | |||
traceIDLength?: number; |
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.
traceIDLength?: number; | |
traceIdDisplayLength?: number; |
@@ -149,7 +150,8 @@ export function TracePageHeaderFn(props: TracePageHeaderEmbedProps & { forwarded | |||
return { ...rest, value: renderer(trace) }; | |||
}); | |||
|
|||
const traceShortID = trace.traceID.slice(0, 7); | |||
const traceIDLength = getConfigValue('traceIDLength'); | |||
const traceShortID = trace.traceID.slice(0, traceIDLength); |
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.
className?: string; | ||
}; | ||
|
||
export function TraceIdDisplayLength({ traceId, className = '' }: Props) { |
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.
export function TraceIdDisplayLength({ traceId, className = '' }: Props) { | |
export function TraceId({ traceId, className = '' }: Props) { |
"DisplayLength" is implementation detail irrelevant to the user.
Signed-off-by: Avinash <[email protected]>
Signed-off-by: Avinash <[email protected]>
@@ -37,7 +37,7 @@ import { getTraceLinks } from '../../../model/link-patterns'; | |||
import './TracePageHeader.css'; | |||
import ExternalLinks from '../../common/ExternalLinks'; | |||
import { getTargetEmptyOrBlank } from '../../../utils/config/get-target'; | |||
import TraceIdDisplayLength from '../../common/TraceIdDisplayLength'; | |||
import TraceId from '../../common/TraceIdDisplayLength'; |
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.
import TraceId from '../../common/TraceIdDisplayLength'; | |
import TraceId from '../../common/TraceId'; |
Signed-off-by: Avinash <[email protected]>
Which problem is this PR solving?
Description of the changes
How was this change tested?
here is the screenshot of the function Full ID
after clicking the Full ID
Checklist
jaeger
:make lint test
jaeger-ui
:npm run lint
andnpm run test