Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[APM] Show errors on the timeline instead of under the transaction #52863

Closed
wants to merge 16 commits into from
Closed

[APM] Show errors on the timeline instead of under the transaction #52863

wants to merge 16 commits into from

Conversation

cauemarcondes
Copy link
Contributor

@cauemarcondes cauemarcondes commented Dec 12, 2019 •
edited
Loading

closes #45619

Show errors on timeline:
Screenshot 2019-12-19 at 15 11 58

  • If the link inside the popover is clicked, it will navigate to the Error detail page, filtering by transaction and trace id.
  • The error count in the summary reflects the number of errors in the timeline.

change the error badge style:
Screenshot 2019-12-18 at 14 58 56

@cauemarcondes cauemarcondes changed the title [APM] Show errors on the timeline instead of under the transaction [WIP][APM] Show errors on the timeline instead of under the transaction Dec 12, 2019
sorenlouv
sorenlouv reviewed Dec 17, 2019
View reviewed changes
...ls/WaterfallWithSummmary/WaterfallContainer/Waterfall/waterfall_helpers/waterfall_helpers.ts
@@ -72,18 +73,33 @@ interface IWaterfallItemBase {
childIds?: Array<IWaterfallItemBase['id']>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looks like childIds is no longer used. Do you agree?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I want to remove it in another PR, because I will also remove other properties.

sorenlouv
sorenlouv reviewed Dec 17, 2019
View reviewed changes
...ls/WaterfallWithSummmary/WaterfallContainer/Waterfall/waterfall_helpers/waterfall_helpers.ts
id: error.error.id,
parentId: error.parent?.id,
serviceName: error.service.name,
message: error.error.log?.message || error.error.exception?.[0]?.message,
Copy link
Member

@sorenlouv sorenlouv Dec 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we are adding more properties (like message) to the top-level waterfallItem which are irrelevant to the other types of items. Maybe we should strip it down to just the things that are required for positioning it on the timeline, and then make an open-ended doc property that contains anything necessary for displaying it.

interface IWaterfallItemBase<T> {
  custom: T;

  /**
   * Duration in us
   */
  duration: number;

  /**
   * offset from first item in us
   */
  offset: number;

  /**
   * skew from timestamp in us
   */
  skew: number;
}

export type IWaterfallItem = IWaterfallItemBase<Transaction> | IWaterfallItemBase<Span>;

My intention is to make the interface as small as possible but I don't know if this is a good approach in practice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sqren is it fine if I do this change in another PR? then I can remove the childIds, getTransactionById and other properties which are not necessary.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

sorenlouv
sorenlouv reviewed Dec 17, 2019
View reviewed changes
...ls/WaterfallWithSummmary/WaterfallContainer/Waterfall/waterfall_helpers/waterfall_helpers.ts
([name, ms]) =>
({
id: name,
name,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having name looks a bit odd. Maybe another reason for keeping the custom data in a specific field:

{
  custom: { customData... },
  offset: ms * 1000,
  docType: 'agentMark'
}

@cauemarcondes cauemarcondes force-pushed the waterfall-erros-timeline branch from 5f09af6 to 227eb30 Compare December 18, 2019 09:13
@cauemarcondes cauemarcondes marked this pull request as ready for review December 18, 2019 13:43
@cauemarcondes cauemarcondes requested a review from a team as a code owner December 18, 2019 13:43
@cauemarcondes cauemarcondes changed the title [WIP][APM] Show errors on the timeline instead of under the transaction [APM] Show errors on the timeline instead of under the transaction Dec 18, 2019
formgeist
formgeist approved these changes Dec 18, 2019
View reviewed changes
Copy link
Contributor

@formgeist formgeist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks terrific! A few nits around the use of styled components > existing or EUI components. Small font size suggestion too.

@cauemarcondes cauemarcondes force-pushed the waterfall-erros-timeline branch from 295fd7c to 9dcdecf Compare December 19, 2019 08:42
formgeist
formgeist reviewed Dec 19, 2019
View reviewed changes
Copy link
Contributor

@formgeist formgeist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a couple of style suggestions. Other than that, I found that the placement of the popover covers the original click-area of the square icon. Is it possible to move the popover offset above the square?

The last thing is whether we should simply set the skewed timestamp, if negative values, to <0 ms? Just to indicate that, yes there's skew, but not necessarily show the full negative timestamp.

@formgeist
Copy link
Contributor

Forgot my redline screenshot
Screenshot 2019-12-19 at 09 54 17

@cauemarcondes
Copy link
Contributor Author

Forgot my redline screenshot
Screenshot 2019-12-19 at 09 54 17

@formgeist have you pulled my last changes? if yes can you send me the link to this waterfall?

@cauemarcondes
Copy link
Contributor Author

Forgot my redline screenshot
Screenshot 2019-12-19 at 09 54 17

Fixed:
Screenshot 2019-12-19 at 10 53 13

@formgeist
Copy link
Contributor

Awesome! Not sure if you haven't gotten there yet, but did you look into repositioning the popover against the square icon? As I mentioned earlier, the popover arrow is covering the square, which means the option to click it again is a little hard. Understandably, the user can click anywhere outside the popover, but it's also a visual thing when comparing it to the other mark tooltips.

Current position

Screenshot 2019-12-19 at 12 24 54

Suggested position

Screenshot 2019-12-19 at 12 24 32

@cauemarcondes
Copy link
Contributor Author

Awesome! Not sure if you haven't gotten there yet, but did you look into repositioning the popover against the square icon? As I mentioned earlier, the popover arrow is covering the square, which means the option to click it again is a little hard. Understandably, the user can click anywhere outside the popover, but it's also a visual thing when comparing it to the other mark tooltips.

Current position

Screenshot 2019-12-19 at 12 24 54

Suggested position

Screenshot 2019-12-19 at 12 24 32

@formgeist WDYT?
Screenshot 2019-12-19 at 15 11 58

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

History

  • 💚 Build #16662 succeeded b0b23fd725a4669cffb35b0d5172650d7b9aed10
  • 💚 Build #16652 succeeded 7aac7ba9388ffd17c142e28a572be9dfc4c10043
  • 💔 Build #16644 failed 9dcdecf3583ec2b7ddc35af51ef1282e9783789c
  • 💔 Build #16636 failed 6a298553ce1d7ea4471260e1d8477fcfe889118c
  • 💚 Build #16517 succeeded 572eadb23303eab842edaa5ca5bee8690bac483a

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@formgeist
Copy link
Contributor

LGTM 🎉

@cauemarcondes cauemarcondes requested review from sorenlouv and a team December 19, 2019 16:52
@cauemarcondes cauemarcondes mentioned this pull request Dec 23, 2019
Merged
@cauemarcondes cauemarcondes force-pushed the waterfall-erros-timeline branch from 9196cbb to 7782d45 Compare January 6, 2020 09:46
@cauemarcondes
Copy link
Contributor Author

@sqren can you review it, please?

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

  • 💚 Build #16697 succeeded 9196cbbd6eb855595486494e623274362f0bddbb
  • 💚 Build #16662 succeeded b0b23fd725a4669cffb35b0d5172650d7b9aed10
  • 💚 Build #16652 succeeded 7aac7ba9388ffd17c142e28a572be9dfc4c10043
  • 💔 Build #16644 failed 9dcdecf3583ec2b7ddc35af51ef1282e9783789c
  • 💔 Build #16636 failed 6a298553ce1d7ea4471260e1d8477fcfe889118c

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

sorenlouv
sorenlouv reviewed Jan 6, 2020
View reviewed changes
...mponents/app/TransactionDetails/WaterfallWithSummmary/WaterfallContainer/Waterfall/index.tsx
@@ -133,6 +135,10 @@ export class Waterfall extends Component<Props> {
const itemContainerHeight = 58; // TODO: This is a nasty way to calculate the height of the svg element. A better approach should be found
const waterfallHeight = itemContainerHeight * waterfall.orderedItems.length;

const marks = waterfall.orderedItems.filter(
item => item.docType === 'error' || item.docType === 'agentMark'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought you ended up deciding not treating agentMark as a docType (because it's not) and just have it has custom payload inside transactions?

sorenlouv
sorenlouv reviewed Jan 6, 2020
View reviewed changes
...ls/WaterfallWithSummmary/WaterfallContainer/Waterfall/waterfall_helpers/waterfall_helpers.ts
const serviceColors = getServiceColors(orderedItems);

// the agentMarks should be added direct inside orderedItems, as it doesnt have parent-child relationship
orderedItems.push(...getAgentMarks(entryTransaction));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little confused about the difference in implementation in this PR and #53756. Which one are you going for?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sorenlouv sorenlouv sorenlouv left review comments

@formgeist formgeist formgeist approved these changes

Assignees
No one assigned
Labels
release_note:enhancement v7.6.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[APM] Show errors on the timeline instead of under the transaction
5 participants
@cauemarcondes @formgeist @elasticmachine @kibanamachine @sorenlouv