Skip to content
This repository has been archived by the owner on Aug 9, 2022. It is now read-only.

Add frontend metrics for Kibana reports #277

Merged
merged 5 commits into from
Jan 4, 2021

Conversation

zhongnansu
Copy link
Member

@zhongnansu zhongnansu commented Dec 24, 2020

Issue #, if available:

Description of changes:

  • Add new api /api/reporting/stats to collect usage metrics and action metrics(per API)
  • Metrics details:
    • count: request count in the last minute
    • total: total request count since the server is up
    • user_error: 4xx error status code in the last minute
    • system_error: all other error status code in the last minute
  • Expected output
      {
      "report": {  
        "create": {  # POST /api/reporting/generateReport
          "count": 0,
          "system_error": 0,
          "user_error": 3,
          "total": 0
        },
        "create_from_definition": {  # POST /api/reporting/gemerateReport/{reportDefinitionId}
          "count": 1,
          "system_error": 0,
          "user_error": 0,
          "total": 1
        },
        "download": {  # GET /api/reporting/generateReport/{reportId}
          "count": 5,
          "system_error": 0,
          "user_error": 0,
          "total": 5
        },
        "list": {  # GET /api/reporting/reports
          "count": 7,
          "system_error": 0,
          "user_error": 0,
          "total": 7
        },
        "info": {  # GET /api/reporting/reports/{reportId}
          "count": 1,
          "system_error": 0,
          "user_error": 0,
          "total": 1
        }
      },
      "report_definition": {
        "create": {  # POST /api/reporting/reportDefinition
          "count": 0,
          "system_error": 0,
          "user_error": 0,
          "total": 0
        },
        "list": {  # GET /api/reporting/reportDefinitions
          "count": 7,
          "system_error": 0,
          "user_error": 0,
          "total": 7
        },
        "info": {  # GET /api/reporting/reportDefinition/{reportDefinitionId}
          "count": 3,
          "system_error": 0,
          "user_error": 0,
          "total": 3
        },
        "update": {   # PUT /api/reporting/reportDefinition/{reportDefinitionId}
          "count": 0,
          "system_error": 0,
          "user_error": 0,
          "total": 0
        },
        "delete": {   # DELETE /api/reporting/reportDefinition/{reportDefinitionId}
          "count": 1,
          "system_error": 0,
          "user_error": 0,
          "total": 1
        }
      },
      "report_source": {   # GET /api/reporting/reportSource/{reportSourceType}
        "list": {
          "count": 3,
          "system_error": 0,
          "user_error": 0,
          "total": 3
        }
      },
      "dashboard": {
        "pdf": {
          "download": {
            "count": 1,
            "total": 1
          }
        },
        "png": {
          "download": {
            "count": 1,
            "total": 1
          }
        }
      },
      "visualization": {
        "pdf": {
          "download": {
            "count": 0,
            "total": 0
          }
        },
        "png": {
          "download": {
            "count": 0,
            "total": 0
          }
        }
      },
      "saved_search": {
        "csv": {
          "download": {
            "count": 4,
            "total": 4
          }
        }
      }
    }
    

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@zhongnansu zhongnansu changed the title add metrics Add metrics for reports downloads Dec 24, 2020
@codecov
Copy link

codecov bot commented Dec 24, 2020

Codecov Report

Merging #277 (e2323d6) into dev (e27146a) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #277      +/-   ##
==========================================
+ Coverage   74.91%   74.94%   +0.02%     
==========================================
  Files          32       32              
  Lines        1754     1764      +10     
  Branches      340      342       +2     
==========================================
+ Hits         1314     1322       +8     
- Misses        435      437       +2     
  Partials        5        5              
Impacted Files Coverage Δ
kibana-reports/server/routes/utils/constants.ts 100.00% <100.00%> (ø)
kibana-reports/server/routes/utils/helpers.ts 50.00% <100.00%> (+11.11%) ⬆️
...port_definitions/report_trigger/report_trigger.tsx 72.13% <0.00%> (-0.95%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e27146a...e2323d6. Read the comment docs.

@zhongnansu zhongnansu marked this pull request as ready for review December 24, 2020 01:53
Copy link
Contributor

@joshuali925 joshuali925 left a comment

Choose a reason for hiding this comment

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

LGTM, minor comments

kibana-reports/server/routes/utils/metricHelper.ts Outdated Show resolved Hide resolved
kibana-reports/server/routes/utils/metricHelper.ts Outdated Show resolved Hide resolved
kibana-reports/server/routes/utils/metricHelper.ts Outdated Show resolved Hide resolved
@abbashus
Copy link
Contributor

We need count, total, client_error, system_error for all the Kibana reporting API endpoints.

  • POST /api/reporting/generateReport
  • GET /api/reporting/generateReport/{reportId}
  • POST /api/reporting/generateReport/{reportDefinitionId}
    .... + 8 others.

The metrics that you posted are business metrics and don't require client_error, system_error. They should only be incremented only when we have successful generation of a report.

@zhongnansu
Copy link
Member Author

We need count, total, client_error, system_error for all the Kibana reporting API endpoints.

  • POST /api/reporting/generateReport
  • GET /api/reporting/generateReport/{reportId}
  • POST /api/reporting/generateReport/{reportDefinitionId}
    .... + 8 others.

The metrics that you posted are business metrics and don't require client_error, system_error. They should only be incremented only when we have successful generation of a report.

What metric do we want to collect from API such as GET /api/reporting/reports (get all reports instance, triggered by refreshing the table on the landing page). And why do we need them?

@abbashus
Copy link
Contributor

abbashus commented Dec 28, 2020

We need count, total, client_error, system_error for all the Kibana reporting API endpoints.

  • POST /api/reporting/generateReport
  • GET /api/reporting/generateReport/{reportId}
  • POST /api/reporting/generateReport/{reportDefinitionId}
    .... + 8 others.

The metrics that you posted are business metrics and don't require client_error, system_error. They should only be incremented only when we have successful generation of a report.

What metric do we want to collect from API such as GET /api/reporting/reports (get all reports instance, triggered by refreshing the table on the landing page). And why do we need them?

Same as count, total, client_error, system_error. To alarm on system_errors if they go above a certain threshold. For example the ES is un-responsive you may get 5XX from ES, and thus this API call will be failing as it's not able to read from index.

@zhongnansu zhongnansu changed the title Add metrics for reports downloads Add metrics for Kibana reports Dec 29, 2020
try {
const metrics = getMetrics();
return response.ok({
body: metrics,
Copy link
Contributor

Choose a reason for hiding this comment

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

np: could simplify to body: getMetrics()?

Copy link
Contributor

@davidcui1225 davidcui1225 left a comment

Choose a reason for hiding this comment

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

LGTM- please address comments

@davidcui1225
Copy link
Contributor

Looks like codecov failed?

@zhongnansu
Copy link
Member Author

Looks like codecov failed?

will add some tests

@abbashus
Copy link
Contributor

  • We don't need a higher level json field usage and business.
  • Missing metric for this API /api/reporting/getReportSource/{reportSourceType}
  • Can you add in the description what metric corresponds to what API, something like
      "info": { # GET /api/reporting/reports --> get all reports instance details
        "count": 0,
        "system_error": 0,
        "user_error": 0,
        "total": 0
      }
  • Please add unit tests

@zhongnansu
Copy link
Member Author

zhongnansu commented Dec 29, 2020

  • We don't need a higher level json field usage and business.
  • Missing metric for this API /api/reporting/getReportSource/{reportSourceType}
  • Can you add in the description what metric corresponds to what API, something like
      "info": { # GET /api/reporting/reports --> get all reports instance details
        "count": 0,
        "system_error": 0,
        "user_error": 0,
        "total": 0
      }
  • Please add unit tests
  • Can we leave those higher-level identifiers, it makes it easier to access the json object, when updating the counters in my logic. Besides, some APIs like list all reports, end up not updating anything to the business metrics.
  • /api/reporting/getReportSource/{reportSourceType} I leave it on purpose because I feel like it doesn't do anything with report or report definition entity. It doesn't interact much with core logic of report, it doesn't call ES-reporting APIs as well. Because it's only used when user are on the "create report definition" page, where a dropdown list of dashboards is rendered.
  • will update desciption

@zhongnansu zhongnansu merged commit 45abee7 into opendistro-for-elasticsearch:dev Jan 4, 2021
@zhongnansu zhongnansu changed the title Add metrics for Kibana reports Add frontend metrics for Kibana reports Jan 4, 2021
zhongnansu added a commit to zhongnansu/kibana-reports that referenced this pull request Jan 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants