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

Bug fix for data consumption prometheus query change #2509

Merged

Conversation

afreen23
Copy link
Contributor

@afreen23 afreen23 commented Aug 28, 2019

Bug

The data consumption card searches for stale metrics and always shows No Prometheus Data Points

Fix

Updated the card with all the latest metrics

Prometheus Queries Overview

  • On the selection of drop down, a few metrics need to polled instead single metric per drop down
  • All metrics are an Instant Vector now with no values hacked into labels.
  • No calculation for addition/sorting needs to be done on UI front, everything is handled by queries.

Subsequent Changes in the UI

  • Due to the change in the Prometheus response for the current metrics , the subsequent individual functions needs to be removed.
  • Replaced the previous individual functions, with a switch case which then fetches the data from generic functions.
  • Enhancement of the chart look and feel

People

Thanx @anmolsachan for reviewing and @jeniawhite for helping with testing and providing the actual metrics for all of the cards.

Screenshots

Screenshot from 2019-09-04 12-09-26
Screenshot from 2019-09-04 12-09-35

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. component/noobaa Related to noobaa-storage-plugin labels Aug 28, 2019
@afreen23
Copy link
Contributor Author

afreen23 commented Aug 28, 2019

@cloudbehl @rawagner @anmolsachan @jeniawhite please review

totalLogicalUsage: 'sum(NooBaa_providers_logical_size)',
},
[ObjectServiceDashboardQuery.PROVIDERS_BY_EGRESS]: {
egress: 'NooBaa_providers_bandwidth_read_size + NooBaa_providers_bandwidth_write_size',
Copy link
Contributor Author

@afreen23 afreen23 Aug 29, 2019

Choose a reason for hiding this comment

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

will change this query to sorting out and all the queries to top 5 in order to be consistent with persistent dashboard

return [{ name: `Total Reads ${totalReads}` }, { name: `Total Writes ${totalWrites}` }];
export const getLegendData: GetLegendData = (response, humanize) => {
const value = _.get(response, 'data.result[0].value[1]', null);
return value ? humanize(Number(value)).string : '';
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to convert to number humanize takes care of it.

Suggested change
return value ? humanize(Number(value)).string : '';
return value ? humanize(value).string : '';

@afreen23 afreen23 force-pushed the data-consumption-query-change branch from c10974a to 14809ab Compare September 3, 2019 09:41
@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 3, 2019
@afreen23 afreen23 force-pushed the data-consumption-query-change branch 2 times, most recently from e12f873 to 3362591 Compare September 3, 2019 09:46
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 3, 2019
- granularized the previous metrics
- extra calculations of the UI are now handled by these queries
  [hence optimizing the performance on UI front]

Signed-off-by: Afreen Rahman <afrahman@redhat.com>
@afreen23 afreen23 force-pushed the data-consumption-query-change branch from 3362591 to 2a9f662 Compare September 3, 2019 12:22
@afreen23
Copy link
Contributor Author

afreen23 commented Sep 4, 2019

@cloudbehl please review it back

Copy link
Contributor

@cloudbehl cloudbehl left a comment

Choose a reason for hiding this comment

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

nit: Can you change it everywhere, to differentiate between function name and the types.

name: string;
unit: string;
yOriginal: number;
type GetChartData = (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, I thought capitalizing would suffice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also its not a props type, rather a function type definition.
We can rename to GetChartDataType if its confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but types are capitalized and function name are camel case, which I know the pattern in general

name?: string,
) => ChartDataPoint[];

type GetDataConsumptionChartData = (

This comment was marked as outdated.

name: string;
unit: string;
yOriginal: number;
type GetChartData = (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, I thought capitalizing would suffice

- Additon of pf4 properties for bar charts
- Tick values fix
- Addition of top margin property in the card body

Signed-off-by: Afreen Rahman <afrahman@redhat.com>
@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 4, 2019
@cloudbehl
Copy link
Contributor

/approve
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 4, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afreen23, cloudbehl

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@cloudbehl
Copy link
Contributor

/test e2e-aws-console

@openshift-merge-robot openshift-merge-robot merged commit c6de8bf into openshift:master Sep 4, 2019
@spadgett spadgett added this to the v4.2 milestone Sep 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/noobaa Related to noobaa-storage-plugin lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants