[apm]Spans missing from waterfall post 6.x upgrade #52385
Conversation
cauemarcondes
force-pushed
the
spans-missing
branch
from
904fd68 to
bb50d7b
Compare
💚 Build Succeeded
|
cauemarcondes
force-pushed
the
spans-missing
branch
from
216ba3c to
4bac51d
Compare
...ns/apm/public/components/app/TransactionDetails/WaterfallWithSummmary/MaybeViewTraceLink.tsx
Show resolved
Hide resolved
...actionDetails/WaterfallWithSummmary/WaterfallContainer/Waterfall/TransactionFlyout/index.tsx
Outdated
Show resolved
Hide resolved
...actionDetails/WaterfallWithSummmary/WaterfallContainer/Waterfall/TransactionFlyout/index.tsx
Outdated
Show resolved
Hide resolved
.../app/TransactionDetails/WaterfallWithSummmary/WaterfallContainer/Waterfall/WaterfallItem.tsx
Outdated
Show resolved
Hide resolved
...thSummmary/WaterfallContainer/Waterfall/waterfall_helpers/__test__/waterfall_helpers.test.ts
Outdated
Show resolved
Hide resolved
...thSummmary/WaterfallContainer/Waterfall/waterfall_helpers/__test__/waterfall_helpers.test.ts
Outdated
Show resolved
Hide resolved
...ls/WaterfallWithSummmary/WaterfallContainer/Waterfall/waterfall_helpers/waterfall_helpers.ts
Show resolved
Hide resolved
| @@ -157,7 +157,8 @@ describe('waterfall_helpers', () => { | |||
| transaction: { | |||
| duration: { us: 49660 }, | |||
| name: 'GET /api', | |||
| id: 'myTransactionId1' | |||
| id: 'myTransactionId1', | |||
| start: { us: 1000 } | |||
There was a problem hiding this comment.
I think start has been deprecated for timestamp.us.
There was a problem hiding this comment.
If the problem is that timestamp.us is not correct on legacy spans please add warning comments when where start is defined to ensure we don't use it by mistake in later versions.
There was a problem hiding this comment.
🤔Shouldn't it be used to create the timestamp when I ran the index migration process? Because the only way to align right the spans is with the start.
There was a problem hiding this comment.
Yes, I'd expect the migration script to have handled this.
There was a problem hiding this comment.
Try asking @graphaelli - I think he worked on the migration script.
...ls/WaterfallWithSummmary/WaterfallContainer/Waterfall/waterfall_helpers/waterfall_helpers.ts
Show resolved
Hide resolved
...ls/WaterfallWithSummmary/WaterfallContainer/Waterfall/waterfall_helpers/waterfall_helpers.ts
Outdated
Show resolved
Hide resolved
...ls/WaterfallWithSummmary/WaterfallContainer/Waterfall/waterfall_helpers/waterfall_helpers.ts
Outdated
Show resolved
Hide resolved
...ls/WaterfallWithSummmary/WaterfallContainer/Waterfall/waterfall_helpers/waterfall_helpers.ts
Outdated
Show resolved
Hide resolved
| ); | ||
| } | ||
| }); | ||
| const waterfallItems = transformTraceItems({ trace, errorsPerTransaction }); |
There was a problem hiding this comment.
transformTraceItems is very generic. I think getWaterfallItems is slightly better. But if you can find something more precise than "waterfall item" I'd be very happy.
That being said, i thought about dropping this entirely and just having regular spans and transactions. Problem with that is that we'll need some custom fields like skew and offset. And instead of mutating the span and transaction docs it seemed better to create a new "thing" which became waterfallItem.
...ls/WaterfallWithSummmary/WaterfallContainer/Waterfall/waterfall_helpers/waterfall_helpers.ts
Show resolved
Hide resolved
| ...toQuery(location.search), | ||
| ...{ | ||
| flyoutDetailTab: undefined, | ||
| waterfallItemId: item ? String(item.id) : undefined |
There was a problem hiding this comment.
I think item.id is always a string, so this might not be necessary anymore (not 100% so please double check).
You should therefore be able to do:
interface IWaterfallItemBase {
- id: string | number;
+ id: string;
| case 'span': | ||
| const parentTransaction = waterfall.getTransactionById( | ||
| currentItem.parentId | ||
| ); |
There was a problem hiding this comment.
What do you think about just adding the parent to each waterfall item? I don't expect this to cost a lot in increased memory since it'll just be pointers to transactions that are already in memory.
There was a problem hiding this comment.
Forgot to say: this will allow us to remove getTransactionById which is good because it decreases the API of waterfall helpers.
| @@ -119,7 +122,7 @@ const createApmApi = () => { | |||
| .add(serviceMapRoute) | |||
|
|
|||
| // Transaction | |||
| .add(transactionByTraceIdRoute); | |||
| .add(rootTransactionByTraceIdRoute); | |||
There was a problem hiding this comment.
This belongs in the "Traces" section
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
|
This was replaced by: #52763 |
When upgrading from 6.x to 7.x the migration assistant will not apply a parent.id to transactions/spans. The UI currently expects parent.id to be available and uses it to build the parent/child relationship. However, if no parent.id exists the trace items (transactions and spans) should instead be ordered by start time.
closes #52252