feat: add correlate span logs in traces waterfall chart #605
Conversation
🦋 Changeset detectedLatest commit: a868d77 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
- Support searching coorelate span log in trace windows Ref: hdx-1181
LYHuang
force-pushed
the
mh/hdx-1181-correlate-logs-for-traces
branch
from
e8fc4a9 to
7c2c758
Compare
| @@ -202,7 +204,14 @@ export default function DBTracePanel({ | |||
| <Text size="sm" c="dark.2" my="sm"> | |||
| Span Details | |||
There was a problem hiding this comment.
This should reflect the correct event type name
There was a problem hiding this comment.
Not sure which part you mean, do you mean Span Details?
| @@ -56,7 +57,7 @@ export default function DBTracePanel({ | |||
|
|
|||
| const [traceRowWhere, setTraceRowWhere] = useQueryState( | |||
| 'traceRowWhere', | |||
There was a problem hiding this comment.
not a huge fan of overloading this query param, esp if the rowWhere is for a log but put in the "traceRowWhere" - the naming wouldn't make sense.
I think it might be better to introduce another query param for logRowWhere? We could rename this query param but it'd technically be a breaking change (technically this schema change is already a breaking change, URLs are supposed to be stable in our app).
There was a problem hiding this comment.
Yeah, I agree with you we should not overload traceRowWhere, it should only return trace relate param, however I think it is a little too overcomplicate to make two queryState for returning focus param.
I think I will change the name to spanRowWhere, it makes more sense since span can be log or trace.
packages/common-utils/src/types.ts
Outdated
| @@ -73,6 +73,7 @@ export const SelectListSchema = z.array(DerivedColumnSchema).or(z.string()); | |||
| export const SortSpecificationSchema = z.intersection( | |||
| RootValueExpressionSchema, | |||
| z.object({ | |||
| valueExpression: z.string().optional(), | |||
There was a problem hiding this comment.
why do we need to add this?
There was a problem hiding this comment.
It get mentioned in lint error,
usually ordering will need to mention column, example:
orderBy: [
{
valueExpression: config.timestampValueExpression,
ordering: 'ASC',
},
],
I am fine to revert this, because I don't think I need order query anymore(data need to sort after fetching anyway).
| // If there's no parent defined, or if the parent doesn't exist, we're a root | ||
| (result.ParentSpanId === '' || !spanIds.has(result.ParentSpanId)) | ||
| ) { | ||
| const isRootNode = |
There was a problem hiding this comment.
I believe this can be a lot more readable if we do the direct boolean expression which I believe is:
const isRootNode = HyperDXEventType === 'span' && type !== SourceKind.Log && !nodeParentSpanId;
There was a problem hiding this comment.
oh cool, good call on this. Also since I am removing HyperDXEventType, this will become:
// root if is trace event and has no parent
const isRootNode = type === SourceKind.Trace && !nodeParentSpanId;
| if (type === SourceKind.Log) logSpanCount += 1; | ||
|
|
||
| // log have dupelicate span id, tag it with -log-couunt | ||
| const nodeSpanId = |
There was a problem hiding this comment.
is this id used anywhere? is it effectively just random and never used further?
There was a problem hiding this comment.
It uses as ref key to get node, it only use for trace event. And for log event, I add -log-count to prevent it overwrite trace event node.
And the reason is, in trace table, all spanId is unique, but in log table, spanId can be duplicate and it is a reference to trace event(more like parentSpanId).
SeverityText: warnwill display as color yellowLog Tab:
Trace Tab:
Ref: hdx-1181