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

[APM] Always render collapsible library stackframes #31320

Merged

Conversation

ogupte
Copy link
Contributor

@ogupte ogupte commented Feb 15, 2019

[APM] closes #28919 by always rendering library stackframes as collapsible, even when there's only 1

screen shot 2019-02-20 at 12 28 16 am

screen shot 2019-02-20 at 12 28 48 am

@ogupte ogupte self-assigned this Feb 15, 2019
@ogupte ogupte requested a review from formgeist February 15, 2019 21:30
@ogupte ogupte requested a review from a team as a code owner February 15, 2019 21:30
@jasonrhodes
Copy link
Member

@ogupte can you explain the difference between library frames and app frames and stack frames again? I think I learned it at one point but I have no memory of the difference so it's hard to know what changes I'm looking at. Thanks! :D

@jasonrhodes
Copy link
Member

From the issue, @formgeist said:

Therefore it's suggested that we always collapse library frames in toggles when there's app frames in the stack trace contents.

Should these all be collapsed by default?

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@ogupte
Copy link
Contributor Author

ogupte commented Feb 16, 2019

@ogupte can you explain the difference between library frames and app frames and stack frames again? I think I learned it at one point but I have no memory of the difference so it's hard to know what changes I'm looking at. Thanks! :D

When Span stackframes are rendered, some of them have the isLibrary flag set on them originating from lines outside of the app code. On render, contiguous app stackframes and library stackframes are grouped together, and displayed differently from each other.

Copy link
Member

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

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

Looks good!
Can you fix the pluralization, so it doesn't say "1 library frames" but "1 library frame"?
Something like this:

{
  "defaultMessage": "{count, plural, one {# library frame} other {# library frames}}",
  "values": {
    "count": stackframes.length
  }
}

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.

LGTM! @sqren already mentioned the pluralization of frame(s) 👍

@formgeist
Copy link
Contributor

formgeist commented Feb 19, 2019

@ogupte Something did occur that stuck with me, so I checked in version 6.6, and it seems like we changed the way we displayed library frames in 6.7 and on. Basically, if there's only library frames available, we choose to uncollapse and show them all since there's no app frames to highlight.

Example of behaviour in 6.6
screencapture-localhost-5601-app-apm-2019-02-19-20_12_08

Current behaviour 6.7+
screencapture-apm-elstc-co-aeu-app-apm-2019-02-19-20_17_17

I guess it must've made it in with the margin tweaks you did previously, just never caught because it's a seemingly rare occurrence (especially when visually testing it). Should we address this in another issue/PR?

@ogupte
Copy link
Contributor Author

ogupte commented Feb 19, 2019

I can address this in this PR, just to clarify: when there are only library frames in the stack trace, we should display them initially expanded.

@formgeist
Copy link
Contributor

Now that I'm looking at it, I actually think we should leave it as we're implementing it now. By always collapsing them, the user will expect this to happen at all time, and it would need their interaction to show them. If we're magically displaying them, I'm not sure they'll be able to tell the toggle is uncollapsed and might mistake them for stack frames. Sorry for the confusion, I was getting carried away by finding that it had changed from the last version.

@ogupte ogupte force-pushed the apm-28919-always-collapse-library-stackframes branch from 1b02e4c to f37e06b Compare February 20, 2019 08:31
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@ogupte
Copy link
Contributor Author

ogupte commented Feb 20, 2019

retest

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@ogupte ogupte dismissed sorenlouv’s stale review February 20, 2019 20:49

pluralization addressed in latest PR

@ogupte ogupte merged commit c8cf805 into elastic:master Feb 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[APM] Always show toggle for library frames in the Stacktrace contents
5 participants