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 tooltip explaining Group ID #60425

Open
wants to merge 7 commits into
base: master
from

Conversation

@mohinderps
Copy link

mohinderps commented Mar 17, 2020

Closes #56413

Summary

Add tooltip explaining Group ID in APM Errors.

Checklist

Transactions Page

Screenshot 2020-03-18 at 6 48 29 PM

Errors Page

Screenshot 2020-03-18 at 6 48 16 PM

@mohinderps mohinderps requested a review from elastic/apm-ui as a code owner Mar 17, 2020
@kibanamachine

This comment has been minimized.

Copy link

kibanamachine commented Mar 17, 2020

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@spalger spalger added the Team:apm label Mar 17, 2020
@elasticmachine

This comment has been minimized.

Copy link
Contributor

elasticmachine commented Mar 17, 2020

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

Copy link
Contributor

formgeist left a comment

@mohinderps Thanks for submitting a PR for this. I only have an ask to implement this using the available EuiIconTip component instead.

Copy link
Contributor

formgeist left a comment

@mohinderps Thanks for reimplementing it using the EuiIconTip. I've noticed that the proposed implementation wasn't quite consistent with our Transactions table tooltips.

Screenshot 2020-03-18 at 09 50 09

I've made some suggested changes to make it consistent.

As an aside; I noticed that the Impact tooltip is actually not using EuiIconTip, so if you're feeling adventurous, that might be something to refactor. 🙂

Finally, could I ask you to add a screenshot of the implementation to the description of the PR? It makes it easier for us to review what's changed.

@formgeist formgeist requested a review from elastic/apm-ui Mar 18, 2020
@mohinderps

This comment has been minimized.

Copy link
Author

mohinderps commented Mar 18, 2020

Thanks @formgeist , I will definitely do the changes.

@mohinderps

This comment has been minimized.

Copy link
Author

mohinderps commented Mar 18, 2020

@formgeist, I have added screenshots this time :)

@sqren
sqren approved these changes Mar 18, 2020
Copy link
Member

sqren left a comment

lgtm

Copy link
Contributor

formgeist left a comment

Lgtm, thanks for updating the impact tooltip as well 👍

@sqren

This comment has been minimized.

Copy link
Member

sqren commented Mar 19, 2020

retest

@sqren

This comment has been minimized.

Copy link
Member

sqren commented Mar 19, 2020

@mohinderps any chance you can fix this issue?

ERROR x-pack failed
      x-pack/legacy/plugins/apm/public/components/app/TransactionOverview/List/index.tsx:7:10 - error TS6133: 'EuiIcon' is declared but its value is never read.

      7 import { EuiIcon, EuiToolTip, EuiIconTip } from '@elastic/eui';
                 ~~~~~~~


      Found 1 error.
@mohinderps mohinderps force-pushed the mohinderps:tooltip-groupId-apm-errors branch from 149b2bb to 97cc6bf Mar 19, 2020
@mohinderps

This comment has been minimized.

Copy link
Author

mohinderps commented Mar 19, 2020

Done @sqren . Also is there a way to test at my local machine ?

@smith

This comment has been minimized.

Copy link
Member

smith commented Mar 19, 2020

Also is there a way to test at my local machine ?

@mohinderps The best way to run jest locally is, cd into the x-pack directory and run:

node scripts/jest.js --testPathPattern=legacy/plugins/apm  
@mohinderps

This comment has been minimized.

Copy link
Author

mohinderps commented Mar 19, 2020

Also is there a way to test at my local machine ?

@mohinderps The best way to run jest locally is, cd into the x-pack directory and run:

node scripts/jest.js --testPathPattern=legacy/plugins/apm  

Thanks @smith , I will make sure I do this next time before creating PR.

@sqren

This comment has been minimized.

Copy link
Member

sqren commented Mar 19, 2020

retest

@formgeist

This comment has been minimized.

Copy link
Contributor

formgeist commented Mar 23, 2020

@elasticmachine merge upstream

@sqren

This comment has been minimized.

Copy link
Member

sqren commented Mar 23, 2020

retest

@sqren

This comment has been minimized.

Copy link
Member

sqren commented Mar 23, 2020

@mohinderps The unit tests are still failing. You can run them like @smith suggested locally.

@mohinderps

This comment has been minimized.

Copy link
Author

mohinderps commented Mar 24, 2020

@mohinderps The unit tests are still failing. You can run them like @smith suggested locally.

Sure.

@mohinderps mohinderps force-pushed the mohinderps:tooltip-groupId-apm-errors branch from 14664b4 to 623a798 Mar 24, 2020
@mohinderps

This comment has been minimized.

Copy link
Author

mohinderps commented Mar 24, 2020

@mohinderps The unit tests are still failing. You can run them like @smith suggested locally.

So I have updated the snapshots. Hope everything works this time.

@smith

This comment has been minimized.

Copy link
Member

smith commented Mar 24, 2020

retest

@smith

This comment has been minimized.

Copy link
Member

smith commented Mar 25, 2020

@elasticmachine merge upstream

@smith

This comment has been minimized.

Copy link
Member

smith commented Mar 25, 2020

@mohinderps not sure what's going on with the snapshots here. I ran jest with -u locally and it made the opposite changes that your last commit did. I tried merging upstream above to see if that would help.

@mohinderps

This comment has been minimized.

Copy link
Author

mohinderps commented Mar 25, 2020

@mohinderps not sure what's going on with the snapshots here. I ran jest with -u locally and it made the opposite changes that your last commit did. I tried merging upstream above to see if that would help.

@smith But then won't the test cases fail again? FYI: I ran this command to update the snapshots.
node scripts/jest.js --testPathPattern=legacy/plugins/apm --updateSnapshot

@smith

This comment has been minimized.

Copy link
Member

smith commented Mar 25, 2020

retest

@kibanamachine

This comment has been minimized.

Copy link

kibanamachine commented Mar 25, 2020

💔 Build Failed

Failed CI Steps


Test Failures

Kibana Pipeline / x-pack-intake-agent / X-Pack Jest Tests.x-pack/legacy/plugins/apm/public/components/app/ErrorGroupOverview/List/__test__.ErrorGroupOverview -> List should render empty state

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has not failed recently on tracked branches


Stack Trace

Error: expect(received).toMatchSnapshot()

Snapshot name: `ErrorGroupOverview -> List should render empty state 1`

- Snapshot
+ Received

@@ -187,11 +187,11 @@
                  >
                    Error message and culprit
                  </span>
                </div>
              </th>
-             <th
+             <td
                className="euiTableHeaderCell"
                data-test-subj="tableHeaderCell_handled_2"
                role="columnheader"
                scope="col"
                style={
@@ -205,11 +205,11 @@
                >
                  <span
                    className="euiTableCellContent__text"
                  />
                </div>
-             </th>
+             </td>
              <th
                aria-live="polite"
                aria-sort="descending"
                className="euiTableHeaderCell"
                data-test-subj="tableHeaderCell_occurrenceCount_3"
    at Object.it (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/x-pack/legacy/plugins/apm/public/components/app/ErrorGroupOverview/List/__test__/List.test.tsx:28:29)
    at Object.asyncJestTest (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/jasmineAsyncInstall.js:102:37)
    at resolve (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:43:12)
    at new Promise (<anonymous>)
    at mapper (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:26:19)
    at promise.then (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:73:41)

Kibana Pipeline / x-pack-intake-agent / X-Pack Jest Tests.x-pack/legacy/plugins/apm/public/components/app/ErrorGroupOverview/List/__test__.ErrorGroupOverview -> List should render with data

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has not failed recently on tracked branches


Stack Trace

Error: expect(received).toMatchSnapshot()

Snapshot name: `ErrorGroupOverview -> List should render with data 1`

- Snapshot
+ Received

@@ -242,11 +242,11 @@
                  >
                    Error message and culprit
                  </span>
                </div>
              </th>
-             <th
+             <td
                className="euiTableHeaderCell"
                data-test-subj="tableHeaderCell_handled_2"
                role="columnheader"
                scope="col"
                style={
@@ -260,11 +260,11 @@
                >
                  <span
                    className="euiTableCellContent__text"
                  />
                </div>
-             </th>
+             </td>
              <th
                aria-live="polite"
                aria-sort="descending"
                className="euiTableHeaderCell"
                data-test-subj="tableHeaderCell_occurrenceCount_3"
    at Object.it (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/x-pack/legacy/plugins/apm/public/components/app/ErrorGroupOverview/List/__test__/List.test.tsx:38:29)
    at Object.asyncJestTest (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/jasmineAsyncInstall.js:102:37)
    at resolve (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:43:12)
    at new Promise (<anonymous>)
    at mapper (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:26:19)
    at promise.then (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:73:41)

Kibana Pipeline / x-pack-intake-agent / X-Pack Jest Tests.x-pack/legacy/plugins/apm/public/components/shared/Stacktrace/__test__.Stackframe when stackframe has source lines should render correctly

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has not failed recently on tracked branches


Stack Trace

Error: expect(received).toMatchSnapshot()

Snapshot name: `Stackframe when stackframe has source lines should render correctly 1`

- Snapshot
+ Received

@@ -126,10 +126,11 @@
        },
      }
    }
  >
    <EuiAccordion
+     arrowDisplay="left"
      buttonContent={
        <FrameHeading
          isLibraryFrame={false}
          stackframe={
            Object {
    at Object.it (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/x-pack/legacy/plugins/apm/public/components/shared/Stacktrace/__test__/Stackframe.test.tsx:22:23)
    at Object.asyncJestTest (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/jasmineAsyncInstall.js:102:37)
    at resolve (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:43:12)
    at new Promise (<anonymous>)
    at mapper (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:26:19)
    at promise.then (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:73:41)

History

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

@mohinderps

This comment has been minimized.

Copy link
Author

mohinderps commented Mar 25, 2020

retest

Hey @smith, I just ran test locally. All of them passed. Should I run the test cases on the entire application instead of just apm?

@smith

This comment has been minimized.

Copy link
Member

smith commented Mar 27, 2020

Hey @smith, I just ran test locally. All of them passed. Should I run the test cases on the entire application instead of just apm?

Yes go ahead and try that. Also merge in the latest master.

I'm also going to tag this for 7.8, since we're in the feature freeze for 7.7.

Thanks so much for all your work on this and sorry for the headaches with the CI.

@smith smith added v7.8.0 and removed v7.7.0 labels Mar 27, 2020
@mohinderps

This comment has been minimized.

Copy link
Author

mohinderps commented Mar 30, 2020

Hey @smith, I just ran test locally. All of them passed. Should I run the test cases on the entire application instead of just apm?

Yes go ahead and try that. Also merge in the latest master.

I'm also going to tag this for 7.8, since we're in the feature freeze for 7.7.

Thanks so much for all your work on this and sorry for the headaches with the CI.

Sure. I will do that. Its totally cool.

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

Successfully merging this pull request may close these issues.

7 participants
You can’t perform that action at this time.