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] Add User agent to trace summary #47526

Merged
merged 2 commits into from
Oct 8, 2019
Merged

Conversation

smith
Copy link
Contributor

@smith smith commented Oct 7, 2019

Fixes #47526

image

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

type UserAgentSummaryItemProps = UserAgent;

const Version = styled('span')`
color: ${theme.textColors.subdued};
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove the subdued text here and make it consistent with how we display the trace % in the other part of the summary.

return (
<EuiToolTip
content={i18n.translate('xpack.apm.transactionDetails.userAgentLabel', {
defaultMessage: 'User agent'
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps just "User agent & version" as the default message as that's what is expected to display.

@smith smith marked this pull request as ready for review October 8, 2019 15:29
@smith smith requested a review from a team as a code owner October 8, 2019 15:29
@smith smith added release_note:skip Skip the PR/issue when compiling release notes v7.5.0 labels Oct 8, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@smith
Copy link
Contributor Author

smith commented Oct 8, 2019

@elasticmachine test this please

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 🎉

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@smith smith merged commit d1b046e into elastic:master Oct 8, 2019
@smith smith deleted the nls/46137/useragent branch October 8, 2019 19:36
@sorenlouv
Copy link
Member

image

It might very well be different in the wild but in our test data 85% of all user agent labels are "Other":

[
  { "key": "Other", "doc_count": 10114314 },
  { "key": "Python Requests", "doc_count": 611706 },
  { "key": "curl", "doc_count": 561747 },
  { "key": "Java", "doc_count": 550257 },
  { "key": "HeadlessChrome", "doc_count": 17 }
]

Should we perhaps omit the user agent if it's "Other" (I expect the agents set it to other by default)?

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@formgeist
Copy link
Contributor

Should we perhaps omit the user agent if it's "Other" (I expect the agents set it to other by default)?

Sure, I think it's reasonable to then omit when there's really nothing important to display in the summary. The actual information can still be found in metadata.

@sorenlouv
Copy link
Member

sorenlouv commented Oct 9, 2019

The actual information can still be found in metadata.

Exactly my thinking. Only makes sense to surface this to the user if it's actually useful..

@smith
Copy link
Contributor Author

smith commented Oct 10, 2019

@sqren @formgeist I'm not sure about hiding other. I grabbed a random transaction and saw this:

{
  "original": "Python/3.7 aiohttp/3.3.2",
  "name": "Other",
  "device": {
    "name": "Other"
  }
}

So perhaps we should keep other and put the original (which could be quite long) in the tooltip. Or maybe we could have the user agent link to the section in the metadata tab. Not sure about how to do it correctly, but I'm certain, that "Other" does not mean "No user agent reported."

@sorenlouv
Copy link
Member

As I understood it, the initial approach was to display short, whitelisted name like "Firefox" (instead of "Mozilla/5.0 (X11; U; OpenBSD i386; en-US; rv:1.9.2.8) Gecko/20101230 Firefox/3.6.8"), "Chrome", "Safari" etc. And then in the tooltip show a slightly longer format including version.

If we couldn't match the user agent we would show "Other" and in the tooltip the original.

@smith
Copy link
Contributor Author

smith commented Oct 10, 2019

As I understood it, the initial approach was to display short, whitelisted name like "Firefox" (instead of "Mozilla/5.0 (X11; U; OpenBSD i386; en-US; rv:1.9.2.8) Gecko/20101230 Firefox/3.6.8"), "Chrome", "Safari" etc. And then in the tooltip show a slightly longer format including version.

If we couldn't match the user agent we would show "Other" and in the tooltip the original.

So if it's not "Other" what do we show in the tooltip? Right now it shows "User agent and version" no matter what.

@formgeist
Copy link
Contributor

I think we should display user.agent.name and user.agent.version like we do now. I think our Opbeans demo is pretty specific when it comes to using virtual user agents rather than a real web browser. We certainly can put it the raw string, but I find it incredibly hard to read. I reckon it's better to keep the field description in the tooltip because we no longer have labels in the summary.

@sorenlouv
Copy link
Member

sorenlouv commented Oct 11, 2019

We can leave it as-is for now.

My main gripe with this is that despite our efforts to trim down and simplify the trace summary it's very easy to add "just another field". All the information is already available below in the metadata tab, so the fields we choose to highlight should be deliberately picked, and we should have a good story for what exactly the user wants to see. Maybe we have that here and I just misunderstood the use cases.

@ogupte ogupte self-assigned this Oct 22, 2019
@ogupte ogupte added the apm:test-plan-done Pull request that was successfully tested during the test plan label Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:test-plan-done Pull request that was successfully tested during the test plan release_note:skip Skip the PR/issue when compiling release notes v7.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants