Skip to content

[APM] Language-specific stacktrace formatting #75924

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
Merged
merged 11 commits into from
Aug 31, 2020
Merged

[APM] Language-specific stacktrace formatting #75924

merged 11 commits into from
Aug 31, 2020

Conversation

@smith
Copy link
Contributor

@smith smith commented Aug 25, 2020 •
edited
Loading

Create stackframe heading renderers for languages as described in #49467.

The intention is to make the frames look like they do in their native language stacktrace output.

The FrameHeading tests include examples.

Rename IStackFrame to Stackframe to match other interfaces.

Fixes #49467.

Screenshots

.NET

Before

image

After

image

this is without classnames. elastic/apm-agent-dotnet#848 is not yet merged, but there are examples with classname in the tests.

Go

Before

image

After

image

Only spacing and indentation changed.

Java

Before

image

After

image

Node

Before

image

After

image

This now includes classname if it's sent but we don't have any Opbeans errors that include this.

Python

Only spacing and indentation changed.

Ruby

Before

image

After

image

RUM

Before

image

After

image

RUM uses the same renderer as Node.

@smith smith marked this pull request as ready for review August 26, 2020 16:46
@smith smith requested a review from a team as a code owner August 26, 2020 16:46
@botelastic botelastic bot added the Team:APM - DEPRECATED Use Team:obs-ux-infra_services. label Aug 26, 2020
@elasticmachine
Copy link
Contributor

elasticmachine commented Aug 26, 2020

Pinging @elastic/apm-ui (Team:apm)

smith added 5 commits August 27, 2020 08:46
@smith
[APM] Fix overlapping transaction names
1ef000e 
...in the table and the header. Did this by adding `word-break: break-all` to them.

Also:

* Rename List to TransactionList
* Add stories for TransactionList and ApmHeader
* Add missing type information to transactions based on sample transaction
*
Fixes elastic#73960.
@smith
Revert "[APM] Fix overlapping transaction names"
d2cf311 
This reverts commit 1ef000e.
@smith
Merge remote-tracking branch 'upstream/master' into nls/lang-trace
7e8d01e
@smith
Merge remote-tracking branch 'upstream/master' into nls/lang-trace
29ed930
@smith
Formatting
21b9f2a
@smith
Copy link
Contributor Author

smith commented Aug 27, 2020

@felixbarny wrote (on internal Elastic Slack):

Some more feedback: before the exception message there should also be the exception type.

I did not change this. We're using the message here. I think if the type is part of the message that's what we should store. So far in this PR we're only adding language-specific formatting for the stackframe headings, not for the messages.

I agree with you it should show the class in the message, but I'm not sure this is the right place to do it.

Could we decrease the padding in-between the frames?

Done.

@formgeist this includes some changes to the spacing and indentation. I'm adding you as a reviewer.

@smith smith requested a review from formgeist August 27, 2020 20:34
sorenlouv
sorenlouv approved these changes Aug 28, 2020
View reviewed changes
Copy link
Member

@sorenlouv sorenlouv left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The spacing and indentation make it look so much better. And the language-specific changes give the stack traces a native feel. Great improvement!

@smith
Copy link
Contributor Author

smith commented Aug 31, 2020

@elasticmachine merge upstream

@felixbarny
Copy link
Member

felixbarny commented Aug 31, 2020

I did not change this. We're using the message here. I think if the type is part of the message that's what we should store. So far in this PR we're only adding language-specific formatting for the stackframe headings, not for the messages.

I think we should store the exception type and message in different fields but then display both.

I agree with you it should show the class in the message, but I'm not sure this is the right place to do it.

You mean this PR is not the right place to do it? I'm fine either way but when not adding it in this PR, could you create a follow-up issue? But if it's a quick thing to do I'm not sure if the overhead of another issue is worth it. Up to you 🙂

@smith
Copy link
Contributor Author

smith commented Aug 31, 2020

You mean this PR is not the right place to do it? I'm fine either way but when not adding it in this PR, could you create a follow-up issue? But if it's a quick thing to do I'm not sure if the overhead of another issue is worth it. Up to you 🙂

Yes I'd like to do it in another PR, since this is about language-specific stackframes and this is making changes to messages. I'll open an issue.

@smith
Copy link
Contributor Author

smith commented Aug 31, 2020

@elasticmachine merge upstream

@smith
Copy link
Contributor Author

smith commented Aug 31, 2020

@felixbarny opened #76296 for message formatting.

@kibanamachine
Copy link
Contributor

kibanamachine commented Aug 31, 2020

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
apm 947 +6 941

async chunks size

id value diff baseline
apm 4.7MB +8.9KB 4.7MB

History

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

@smith smith merged commit 2486a71 into elastic:master Aug 31, 2020
@smith smith deleted the nls/lang-trace branch August 31, 2020 15:59
@smith smith mentioned this pull request Aug 31, 2020
Merged
smith added a commit to smith/kibana that referenced this pull request Aug 31, 2020
@smith
[APM] Language-specific stacktrace formatting (elastic#75924)
b11b942 
* [APM] Language-specific stacktrace formatting

* Add todos

* more

* add at prefix for java

* [APM] Fix overlapping transaction names

...in the table and the header. Did this by adding `word-break: break-all` to them.

Also:

* Rename List to TransactionList
* Add stories for TransactionList and ApmHeader
* Add missing type information to transactions based on sample transaction
*
Fixes elastic#73960.
# Conflicts:
#	x-pack/plugins/apm/typings/es_schemas/raw/error_raw.ts
smith added a commit that referenced this pull request Aug 31, 2020
@smith
[APM] Language-specific stacktrace formatting (#75924) (#76321)
f5b2654
@formgeist formgeist mentioned this pull request Sep 1, 2020
Merged
9 tasks
@cauemarcondes cauemarcondes self-assigned this Oct 12, 2020
@cauemarcondes
Copy link
Contributor

cauemarcondes commented Oct 12, 2020

New bug: #80152
Other than that it works great.
Chrome ✅
FF ✅
Safari ✅

@cauemarcondes cauemarcondes added the apm:test-plan-done Pull request that was successfully tested during the test plan label Oct 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sorenlouv sorenlouv sorenlouv approved these changes

@formgeist formgeist Awaiting requested review from formgeist

Assignees

@cauemarcondes cauemarcondes

Labels

apm:test-plan-done Pull request that was successfully tested during the test plan release_note:enhancement Team:APM - DEPRECATED Use Team:obs-ux-infra_services. v7.10.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[APM] Language specific stack frame formatting

6 participants

@smith @elasticmachine @felixbarny @kibanamachine @cauemarcondes @sorenlouv