-
Notifications
You must be signed in to change notification settings - Fork 11
fix: waterfall undefined strings #803
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
Conversation
Codecov Report
@@ Coverage Diff @@
## main #803 +/- ##
=======================================
Coverage 85.41% 85.42%
=======================================
Files 792 792
Lines 16178 16180 +2
Branches 2063 2067 +4
=======================================
+ Hits 13819 13821 +2
Misses 2328 2328
Partials 31 31
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
@@ -115,7 +115,6 @@ export class SequenceChartComponent implements OnChanges { | |||
id: segment.id, | |||
start: segment.start - minStart, | |||
end: segment.end - minStart, | |||
label: segment.label, |
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.
I couldn't see anywhere this was used, so removed it. cc: @anandtiwary to confirm this is okay.
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 left a comment to check beforehand to merge it.
} | ||
|
||
private buildTitleString(trace: Trace): string { | ||
if (trace.spans?.length === 1) { | ||
const entrySpan = trace.spans[0]; | ||
|
||
return `${entrySpan.serviceName as string} ${entrySpan.displaySpanName as string}`; | ||
return `${entrySpan.serviceName as string} ${(entrySpan.displaySpanName as string) ?? ''}`.trim(); |
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.
Wouldn't it be correct to do the trim() before generating the 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.
In this case since we also have a space in the middle of the template, I trim()
once the complete string is built to remove any space between (since there might not be a displaySpanName
).
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.
LGTM
Description
This fixes undefined strings in several places for waterfall.