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] Break down transaction table api removing the sparklines #88946

Merged
merged 45 commits into from
Feb 12, 2021

Conversation

cauemarcondes
Copy link
Contributor

@cauemarcondes cauemarcondes commented Jan 21, 2021

closes #88938

To dos:

  • Remove sparkline timeseries from API, the avg values must stay because it should be sortable on the table.
  • Remove pagination from API
  • Create a new API that returns the sparkline timeseries based on the service names received as property

/api/apm/services/opbeans-java/transactions/groups/primary_statistics:

{
   "transactionGroups":[
      {
         "name":"DispatcherServlet#doGet",
         "transactionType":"request",
         "latency":{
            "value":2293337.6551724137
         },
         "throughput":{
            "value":1.933331185187572
         },
         "errorRate":{
            "value":0.06896551724137931
         },
         "impact":null
      }
   ],
   "totalTransactionGroups":1,
   "isAggregationAccurate":true
}

/api/apm/services/opbeans-java/transactions/groups/comparison_statistics:

{
   "DispatcherServlet#doGet":{
      "latency":{
         "timeseries":[
            {
               "x":1611565320000,
               "y":42067.8
            },
            ...
         ]
      },
      "throughput":{
         "timeseries":[
            {
               "x":1611565320000,
               "y":0.3333329629633745
            },
            ...
         ]
      },
      "errorRate":{
         "timeseries":[
            {
               "x":1611565320000,
               "y":0
            },
            ...
         ]
      }
   }
}
transactions.table.mov

@cauemarcondes cauemarcondes force-pushed the apm-transaction-table-api branch from ec05dc2 to 22b3729 Compare January 25, 2021 08:39
@cauemarcondes cauemarcondes changed the title apm-transaction-table-api [APM] Break down transaction table api removing the sparklines Jan 25, 2021
@cauemarcondes cauemarcondes marked this pull request as ready for review January 25, 2021 09:23
@cauemarcondes cauemarcondes requested a review from a team as a code owner January 25, 2021 09:23
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Jan 25, 2021
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@smith smith left a comment

Choose a reason for hiding this comment

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

Works well.

Copy link
Member

@dgieselaar dgieselaar left a comment

Choose a reason for hiding this comment

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

There's now a bug where comparison data is not refetched if the UI filters change or the latency aggregation type changes. Requesting changes to make sure this is not accidentally merged.

@cauemarcondes
Copy link
Contributor Author

There's now a bug where comparison data is not refetched if the UI filters change or the latency aggregation type changes. Requesting changes to make sure this is not accidentally merged.

I see now a real reason of having the requesId, if you change the latencyAggregationType and the same list of transaction is returned the comparison statistics won't be refetched. Adding it back, and fixing the bug.

@cauemarcondes cauemarcondes requested a review from a team as a code owner February 10, 2021 06:31
@botelastic botelastic bot added the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label Feb 10, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@cauemarcondes
Copy link
Contributor Author

@dgieselaar I just fixed the bug.

primary-comparison-stats.mov

@cauemarcondes cauemarcondes added the auto-backport Deprecated - use backport:version if exact versions are needed label Feb 10, 2021
urlParams: { start, end, latencyAggregationType },
} = useUrlParams();

const { data = INITIAL_STATE, status, requestId } = useFetcher(
Copy link
Member

Choose a reason for hiding this comment

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

This file is getting quite big. It might help if you extract the data fetching, including some of the pagination logic eg:

const {
    currentPagePrimaryStatistics,
    primaryStatisticsStatus,
    totalCount,
    requestId,
  } = usePrimaryStatisticsFetcher({ serviceName, pageIndex, sort });

You can do the same with the comparison statistics

@sorenlouv
Copy link
Member

lgtm. I can't reproduce the bug that @dgieselaar found previously.

Copy link
Contributor

@shahzad31 shahzad31 left a comment

Choose a reason for hiding this comment

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

UX App, only tests for useFetcher are updated, LGTM !!

@cauemarcondes any idea why that hook needs requestId now?

@dgieselaar
Copy link
Member

@elasticmachine merge upstream

@dgieselaar dgieselaar self-assigned this Feb 12, 2021
@dgieselaar
Copy link
Member

I've reverted both requestId changes - the way it was implemented was that requestId would change whenever a new request starts. The previous implementation was that it would only change when a new request ends. I've also added support for transaction duration metrics. Once CI is green I'll merge.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
apm 1730 1731 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
apm 5.2MB 5.2MB +2.0KB

History

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

@dgieselaar dgieselaar enabled auto-merge (squash) February 12, 2021 10:48
@dgieselaar dgieselaar disabled auto-merge February 12, 2021 10:56
@dgieselaar dgieselaar merged commit 2fcf2a9 into elastic:master Feb 12, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Feb 12, 2021
…ic#88946)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Dario Gieselaar <dario.gieselaar@elastic.co>
@kibanamachine
Copy link
Contributor

Backport result

{"level":"info","message":"POST https://api.github.com/graphql (status: 200)"}
{"level":"info","message":"POST https://api.github.com/graphql (status: 200)"}
{"meta":{"labels":["Team:apm","Team:uptime","apm:comparison","auto-backport","release_note:enhancement","v7.12.0"],"branchLabelMapping":{"^v8.0.0$":"master","^v7.12.0$":"7.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"},"existingTargetPullRequests":[]},"level":"info","message":"Inputs when calculating target branches:"}
{"meta":["7.x"],"level":"info","message":"Target branches inferred from labels:"}
{"meta":{"killed":false,"code":2,"signal":null,"cmd":"git remote rm kibanamachine","stdout":"","stderr":"error: No such remote: 'kibanamachine'\n"},"level":"info","message":"exec error 'git remote rm kibanamachine':"}
{"meta":{"killed":false,"code":2,"signal":null,"cmd":"git remote rm elastic","stdout":"","stderr":"error: No such remote: 'elastic'\n"},"level":"info","message":"exec error 'git remote rm elastic':"}
{"level":"info","message":"Backporting [{\"sourceBranch\":\"master\",\"targetBranchesFromLabels\":[\"7.x\"],\"sha\":\"2fcf2a91cc870c4dfe52d1107885cefb40e0100e\",\"formattedMessage\":\"[APM] Break down transaction table api removing the sparklines (#88946)\",\"originalMessage\":\"[APM] Break down transaction table api removing the sparklines (#88946)\\n\\nCo-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>\\r\\nCo-authored-by: Dario Gieselaar <dario.gieselaar@elastic.co>\",\"pullNumber\":88946,\"existingTargetPullRequests\":[]}] to 7.x"}

Backporting to 7.x:
{"level":"info","message":"Backporting via filesystem"}
{"level":"info","message":"Creating PR with title: \"[7.x] [APM] Break down transaction table api removing the sparklines (#88946)\". kibanamachine:backport/7.x/pr-88946 -> 7.x"}
{"level":"info","message":"POST /repos/elastic/kibana/pulls - 201 in 1491ms"}
{"level":"info","message":"Adding assignees to #91284: cauemarcondes"}
{"level":"info","message":"POST /repos/elastic/kibana/issues/91284/assignees - 201 in 1116ms"}
{"level":"info","message":"Adding labels: backport"}
{"level":"info","message":"POST /repos/elastic/kibana/issues/91284/labels - 200 in 401ms"}
View pull request: https://github.com/elastic/kibana/pull/91284

dgieselaar added a commit that referenced this pull request Feb 12, 2021
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Dario Gieselaar <dario.gieselaar@elastic.co>

Co-authored-by: Cauê Marcondes <55978943+cauemarcondes@users.noreply.github.com>
Co-authored-by: Dario Gieselaar <dario.gieselaar@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:comparison auto-backport Deprecated - use backport:version if exact versions are needed release_note:enhancement Team:APM All issues that need APM UI Team support Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[APM] Break down transaction table api removing the sparklines
7 participants