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.

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

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

Closed
wants to merge 10 commits into from

Conversation

cauemarcondes
Copy link
Contributor

closes #45619

}

interface IWaterfallItemTransaction extends IWaterfallItemBase {
transaction: Transaction;
trace: Transaction; // TODO: check this name
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 I removed most of the properties from getTransactionItem and getSpanItem, and replaced it by the real Transaction and Span. But I changed the name to trace because there are some common properties between them. So I don't have to check the type every time. I can simply do item.trace.timestamp.us.
I'd like to have your insights about this change and about the name trace, don't know if this is the best.

Copy link
Member

@sorenlouv sorenlouv Dec 12, 2019

Choose a reason for hiding this comment

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

I think doc is better than trace. trace is normally a list (tree?) or transactions/spans. Whereas this is the actual elasticsearch document for either a span or a transaction.
It also aligns well with docType.

* start timestamp in us
*/
timestamp: number;
parent?: IWaterfallItem;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the parent here to remove the getTransactionById from the returned object.

// the traceroot cannot be found, so we cannot link to it
if (!waterfall.traceRoot) {
const { rootTransaction } = waterfall;
// the traceRootTransaction cannot be found, so we cannot link to it
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// the traceRootTransaction cannot be found, so we cannot link to it
// the rootTransaction cannot be found, so we cannot link to it

return <EuiIcon type="globe" />;
}

// icon for other transactions
return <EuiIcon type="merge" />;
// icon for RUM agent transactions
Copy link
Member

Choose a reason for hiding this comment

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

Was this moved on purpose?

}

interface IWaterfallItemTransaction extends IWaterfallItemBase {
transaction: Transaction;
trace: Transaction; // TODO: check this name
Copy link
Member

@sorenlouv sorenlouv Dec 12, 2019

Choose a reason for hiding this comment

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

I think doc is better than trace. trace is normally a list (tree?) or transactions/spans. Whereas this is the actual elasticsearch document for either a span or a transaction.
It also aligns well with docType.

@elasticmachine
Copy link
Contributor

💔 Build Failed

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[APM] Show errors on the timeline instead of under the transaction
3 participants