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] UI changes to trace summary (above waterfall/timeline) #43247

Closed
1 of 6 tasks
katrin-freihofner opened this issue Aug 14, 2019 · 39 comments · Fixed by #44842
Closed
1 of 6 tasks

[APM] UI changes to trace summary (above waterfall/timeline) #43247

katrin-freihofner opened this issue Aug 14, 2019 · 39 comments · Fixed by #44842
Assignees
Labels
enhancement New value added to drive a business result good first issue low hanging fruit Team:APM All issues that need APM UI Team support v7.5.0

Comments

@katrin-freihofner
Copy link
Contributor

katrin-freihofner commented Aug 14, 2019

This issue is about a small UI update for the information grid within the Transaction sample panel. The goal is to remove the generic grid of label/values to a single line of values without labels. (Discussion initiated by: #42357)

Sub tasks:

Implementation notes:

  • use euiText--small
  • I used the euiColorMediumShade #98A2B3 for the pipes |
  • the method (GET,...) should be bold

It should look similar to:
Screenshot 2019-08-27 at 12 20 16

This work does not include the arrows which is specified in this issue #35574

@katrin-freihofner
Copy link
Contributor Author

@sqren, @nehaduggal should we stay with Transaction sample as a headline, or is there a better solution?

@katrin-freihofner katrin-freihofner added the Team:APM All issues that need APM UI Team support label Aug 14, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui

@dgieselaar
Copy link
Member

dgieselaar commented Aug 14, 2019

This is from Stackdriver:

image

What I really like about this is that if you click on a dot, it's much more clear that you're going to inspect the trace that that dot represents. To me it's really hard to (intuitively) make the connection between clicking on a bucket and then inspecting a sample in that bucket. And perhaps having a scatter plot opens up more visualisation possibilities, e.g. color by result, or size by the number of child spans.

(Probably out of scope though 😀)

@sorenlouv
Copy link
Member

sorenlouv commented Aug 14, 2019

To me it's really hard to (intuitively) make the connection between clicking on a bucket and then inspecting a sample in that bucket

I didn't intuitively understand the histogram in the beginning, whereas the dots do make sense immediately. It also gives the user a second dimension (time) that they can select from.
It is a little less clean, and the histogram is a bit better at showing the distribution (I think). And many overlapping dots could make the navigation a little awkward.

But I'm definitely open to discussing whether it could replace the histogram.

@sorenlouv
Copy link
Member

Another problem with the current histogram: it is the only viz where x-axis is not time. This is confusing initially I think.

@katrin-freihofner
Copy link
Contributor Author

I'm biased - I can clearly see that ;)
I like the idea of having another iteration about the bucket chart. It obviously has some drawbacks. But I would also like to move forward with the small changes to the info-grid above the timeline. Is it ok if we move the chart discussion to a new issue?

@sorenlouv sorenlouv added [zube]: Inbox enhancement New value added to drive a business result good first issue low hanging fruit and removed [zube]: Inbox labels Aug 14, 2019
@sorenlouv
Copy link
Member

@katrin-freihofner We currently use a custom component StickyProperties to build the list but agree we should use the EUI description list if possible. This change will affect the lists on the following pages:

  • Transaction details page (the one we are talking about here)
  • Error details page
  • Transaction flyout
  • Span flyout

Is that ok? If so, I think this looks good, and the issue can be moved to the APM UI board.

@katrin-freihofner
Copy link
Contributor Author

@sqren yes, actually that's perfect!

@nehaduggal
Copy link

How about we call it Trace sample instead of Transaction sample? The waterfall essentially surfaces a trace with one or more transactions so calling it Transaction sample can be misleading.

@sorenlouv sorenlouv changed the title [APM] transaction summary ui update [APM] Timeline/waterfall summary update Aug 17, 2019
@sorenlouv
Copy link
Member

@katrin-freihofner Did you see this related issue?
#29928

Do you think the two issues should be kept separate, or do you want to combine them?

@katrin-freihofner
Copy link
Contributor Author

@sqren no, I have not been aware of this issue. Thank you. I'm fine with combining them.

@katrin-freihofner
Copy link
Contributor Author

katrin-freihofner commented Aug 19, 2019

Summarising our discussion in slack:

Instead of the pretty big description list/grid we want to try to fit the information in one line beneath the headline, like:
Trace sample
19. Aug 2019 09:50:12 | 153 ms | 10% of trace | https://myurl.com | <EuiBadge>5XX</EuiBadge>

I suggest to move the labels to tooltips. Additionally, as discussed in this issue #29928 we should use the EUI Badge for 4XX and 5XX response codes.

@sqren, @roncohen, @nehaduggal, @dgieselaar should we drop the error info if there are no errors? And can we move user ID, device and agent to the meta info?

@sorenlouv
Copy link
Member

@katrin-freihofner

Instead of the pretty big description list/grid we want to try to fit the information in one line beneath the headline

++

I suggest to move the labels to tooltips.

Also fine with me. For many fields we have both a label (eg. "Duration") and a field name ("transaction.duration.us")
Screen Shot 2019-08-19 at 12 47 39

I'm okay with moving the label into the tooltip, and thus getting rid of the field name. If people want to know where the value resides in elasticsearch they can find that in the metadata.

should we drop the error info if there are no errors

I think we should. I don't see any good reasons to show "No errors for trace". Wouldn't this be the expected default? And then draw attention to errors with a red badge if there are any.

And can we move user ID, device and agent to the meta info?

We already show user.id but none of the user agent fields. We probably want to show these:
Screen Shot 2019-08-19 at 13 03 58

@katrin-freihofner
Copy link
Contributor Author

I updated the description according the our discussion. @sqren I think this is ready for implementation. What's your opinion?

@sorenlouv
Copy link
Member

sorenlouv commented Aug 20, 2019

@katrin-freihofner I updated the description with some more sub tasks. LMK if that aligns with your thinking.

Questions:

  • How should we handle very long urls? I think we could truncate above, say 50 chars, and then show the full url in a tooltip.
  • I don't see http method or http status code in the screenshot. I think it would be great if we could somehow "group" those together with the url graphically.

Swagger.io (API tool) groups http method and url like this:
Screen Shot 2019-08-20 at 13 08 01

Insomnia (REST client) groups http details like this:
Screen Shot 2019-08-20 at 13 07 11

I think the use of badges can help the information stand out more clearly. Right now all the information has the same formatting which makes it harder to distinguish.

@katrin-freihofner
Copy link
Contributor Author

@sqren that is perfect, thank you.
Regarding your questions:

@sorenlouv
Copy link
Member

@katrin-freihofner What do you think about something like this?
https://codesandbox.io/s/eui-badges-h8vhj

@katrin-freihofner
Copy link
Contributor Author

@sqren I would use the hollow style for the url but otherwise - yes, that's a good idea.
Screenshot 2019-08-20 at 14 50 36

@sorenlouv
Copy link
Member

Just brainstorming here, but instead of using | as dividers could we use the hollow badges? And additionally combine the duration and percentage of trace in a single badge (that also makes the percentage tangible)

200:
Screen Shot 2019-08-20 at 15 06 48

300:
Screen Shot 2019-08-20 at 15 08 30

500:
Screen Shot 2019-08-20 at 15 06 36

@katrin-freihofner katrin-freihofner removed their assignment Aug 27, 2019
@sorenlouv sorenlouv changed the title [APM] Timeline/waterfall summary update [APM] Trace summary update Aug 28, 2019
@sorenlouv sorenlouv changed the title [APM] Trace summary update [APM] UI changes to trace summary (above waterfall/timeline) Aug 28, 2019
@smith smith self-assigned this Aug 29, 2019
@smith
Copy link
Contributor

smith commented Aug 29, 2019

For non-HTTP requests, we talked on Slack about doing the following:

  • Leaving out the URL and method
  • Using transaction.result instead of http.status_code

(A non-HTTP reques is a request that does not have a http key in its transaction, so things like background or batch jobs, or possibly other network protocols like grpc or thrift that aren't going over HTTP.)

You can see some non-http requests in the sample data under apm-server > Process Pending and on opbeans-python if you change the transaction type to "celery" (a background job processing framework).

The celery traces have a SUCCESS or FAILURE result while the apm-server one has none.

We can probably apply the same colors as 2xx/5xx to SUCCESS/FAILURE but I think a result could be anything.

@katrin-freihofner let me know if this sounds good or if you would like any more changes to the non-HTTP requests.

@katrin-freihofner
Copy link
Contributor Author

@smith, in general, this sounds good to me. As far as I know, we did not pick a specific success color yet. Looking at the Eui Badges (https://elastic.github.io/eui/#/display/badge) I propose to go for secondary.

@sorenlouv
Copy link
Member

sorenlouv commented Aug 30, 2019

We can probably apply the same colors as 2xx/5xx to SUCCESS/FAILURE but I think a result could be anything.

Yes, transaction.result could be any string so I don't think we should attempt to apply colors to it.

@smith
Copy link
Contributor

smith commented Aug 30, 2019

@katrin @sqren a few questions:

The only thing left in the StickyTransactionProperties after we move everything to the single line and metadata is “Errors”, which we only show if there are related errors.

It seems like it would look weird if that label and error count were just sitting there alone, so where and how should we be displaying the “[ N ] Related Errors”?

For the tooltips when hovering over the values on the single-line summary, there’s some comments on the issue:

Would the tooltip only show the label name or the value too?

The tooltip will only show the value.

Does that mean when hovering over “20ms” I would see “transaction.duration.us” in the tooltip or “Duration”? Not sure about what we mean by “name” and “value” in this context.

and also:

I’m okay with moving the label into the tooltip, and thus getting rid of the field name. If people want to know where the value resides in elasticsearch they can find that in the metadata.

But the metadata tab only shows some values and does not include the span.duration or transaction.result for example. Should we add those to the metadata as well?

@sorenlouv
Copy link
Member

@smith fyi: @dgieselaar has a fix for #42357 in #44274.

I think we should get entirely rid of StickyTransactionProperties and the error count should be displayed on the single line.

@sorenlouv
Copy link
Member

sorenlouv commented Sep 2, 2019

But the metadata tab only shows some values and does not include the span.duration or transaction.result for example. Should we add those to the metadata as well?
@smith

I added a todo for adding the transaction.result to metadata.
Regarding span.duration: we are currently only retiring the StickyTransactionProperties component. There is still a StickySpanProperties component when the user clicks on a span. We haven't decided what to do with that, and this issue shouldn't be blocked by that.

@smith
Copy link
Contributor

smith commented Sep 3, 2019

we are currently only retiring the StickyTransactionProperties component. There is still a StickySpanProperties component when the user clicks on a span. We haven't decided what to do with that, and this issue shouldn't be blocked by that.

@sqren StickyTransactionProperties is also used on the flyout, so I'm leaving it alone but removing it from the transaction page.

@sorenlouv
Copy link
Member

sorenlouv commented Sep 3, 2019

@smith Makes sense to leave it for now. We can do that as a follow-up.

@katrin-freihofner
Copy link
Contributor Author

@smith regarding the tooltip I think it is fine to show Duration. transaction.duration.us is for a more advanced use case and as far as I know, it will be visible in the flyout.

smith added a commit to smith/kibana that referenced this issue Sep 5, 2019
Replace the `StickyTransactionProperties` with a trace summary that puts everything on one line.

Fixes elastic#43247.
@smith
Copy link
Contributor

smith commented Sep 5, 2019

@dgieselaar @sqren these things seem to be in conflict:

The timestamp and duration labels are relatively long...

The summary relatively quickly starts wrapping...

and

URL shouldn't be truncated so aggressively

In addition to status code we should also show status text, eg "200 Success"

@katrin-freihofner would you mind taking a look to see if there's changes we can make to present more information and prevent it from wrapping so much?

smith added a commit to smith/kibana that referenced this issue Sep 6, 2019
Replace the `StickyTransactionProperties` with a trace summary that puts everything on one line.

Fixes elastic#43247.
smith added a commit to smith/kibana that referenced this issue Sep 9, 2019
Replace the `StickyTransactionProperties` with a trace summary that puts everything on one line.

Fixes elastic#43247.
smith added a commit that referenced this issue Sep 9, 2019
Replace the `StickyTransactionProperties` with a trace summary that puts everything on one line.

Fixes #43247.
smith added a commit to smith/kibana that referenced this issue Sep 9, 2019
Replace the `StickyTransactionProperties` with a trace summary that puts everything on one line.

Fixes elastic#43247.
smith added a commit that referenced this issue Sep 10, 2019
Replace the `StickyTransactionProperties` with a trace summary that puts everything on one line.

Fixes #43247.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New value added to drive a business result good first issue low hanging fruit Team:APM All issues that need APM UI Team support v7.5.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants