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

Change grafana dashboards for number of workspaces/users to be graphs with axes #14588

Merged
merged 9 commits into from
Sep 25, 2019

Conversation

tomgeorge
Copy link
Contributor

What does this PR do?

Changes the "Number of Users" and "Number of Workspaces" Grafana Panels to show axes, and add nice labels to the legend

What issues does this PR fix or reference?

#14155

Change grafana dashboards for number of workspaces/users to be graphs that show axes, legends

Testing

oc apply -f deploy/openshift/templates/monitoring/grafana-dashboards.yaml -f deploy/openshift/templates/monitoring/grafana-datasources.yaml
oc process -f deploy/openshift/templates/monitoring/che-monitoring.yaml | oc apply -f -
# ensure CHE_METRICS_ENABLED=true

Release Notes

Docs PR

@che-bot
Copy link
Contributor

che-bot commented Sep 18, 2019

Can one of the admins verify this patch?

1 similar comment
@che-bot
Copy link
Contributor

che-bot commented Sep 18, 2019

Can one of the admins verify this patch?

@che-bot che-bot added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/enhancement A feature request - must adhere to the feature request template. labels Sep 18, 2019
"legend": {
"avg": false,
"current": false,
"max": false,
Copy link
Member

Choose a reason for hiding this comment

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

could we have it set to true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

"avg": false,
"current": false,
"max": false,
"min": false,
Copy link
Member

Choose a reason for hiding this comment

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

could we have it set to true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Chagned

"legend": {
"avg": false,
"current": false,
"max": false,
Copy link
Member

Choose a reason for hiding this comment

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

could we have it set to true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Chagned

"avg": false,
"current": false,
"max": false,
"min": false,
Copy link
Member

Choose a reason for hiding this comment

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

could we have it set to true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

@skabashnyuk
Copy link
Contributor

skabashnyuk commented Sep 19, 2019

May I suggest an alternative approach?
Знімок екрана  о 08 47 28

I would like to propose

  • leave area 1 as an indicator area. With values that are simple to read.
    Like just numbers without graphs.
  • Move a total number of workspaces to workspace area - 2. Here we can make it as a graph with min, max, avg values
  • Make a user panel somewhere below. We can add total number of uses, later we can add online aka active users number.

@ibuziuk
Copy link
Member

ibuziuk commented Sep 19, 2019

@skabashnyuk sounds like a good idea @tomgeorge wdyt ?

@tomgeorge
Copy link
Contributor Author

I can make that change

@tomgeorge
Copy link
Contributor Author

Pushed the suggested changes

cc @ibuziuk @skabashnyuk

@skabashnyuk
Copy link
Contributor

Can you provide a screenshot of the result?

@skabashnyuk
Copy link
Contributor

−415 lines removed. Are you sure I have the latest #14578 changes ?

@tomgeorge
Copy link
Contributor Author

No looks like I messed that up. I need to fix this branch.

… that show axes, legends

Signed-off-by: Tom George <tg82490@gmail.com>
Signed-off-by: Tom George <tg82490@gmail.com>
Moved # of workspaces graph to the workspaces row
Moved # of users graph to a new Users row

Signed-off-by: Tom George <tg82490@gmail.com>
@tomgeorge tomgeorge changed the base branch from master to 7.0.x September 19, 2019 14:30
@tomgeorge
Copy link
Contributor Author

GH was showing only 2 commits in the PR view compared to master...tomgeorge:che-14155 which was showing 3 commits. Looks like the state got out of whack on the GH side. Changing the base branch to 7.0.x and back to master seems to show the correct diff. Sorry!

@tomgeorge
Copy link
Contributor Author

tomgeorge commented Sep 19, 2019

General row
general-panel

Workspaces row
workspaces-panel

Users row
users-panel

edit: s/panel/row

@@ -3721,7 +3727,7 @@ data:
"value": "null"
}
],
"valueName": "current"
"valueName": "avg"
Copy link
Member

Choose a reason for hiding this comment

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

@tomgeorge so, currently the Number of Users / Number of Workspaces will show the average for the given timeframe? Not sure if it makes sense... I would opt for Current number of users & workspaces

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, changed back to current.

Signed-off-by: Tom George <tg82490@gmail.com>
Signed-off-by: Tom George <tg82490@gmail.com>
Copy link
Member

@ibuziuk ibuziuk left a comment

Choose a reason for hiding this comment

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

LGTM
@skabashnyuk could you please provide code-owner review / approval ?

Copy link
Contributor

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@skabashnyuk skabashnyuk left a comment

Choose a reason for hiding this comment

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

Looks good to me.

},
"yaxes": [
{
"format": "short",
Copy link
Contributor

Choose a reason for hiding this comment

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

"decimals": 0,
maybe it makes sense to add "decimals=0"

},
"yaxes": [
{
"format": "short",
Copy link
Contributor

Choose a reason for hiding this comment

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

what about "decimals": 0, ?

Signed-off-by: Tom George <tg82490@gmail.com>
@tomgeorge
Copy link
Contributor Author

tomgeorge commented Sep 24, 2019

@skabashnyuk I changed the formatting of those panels to have zero precision

@ibuziuk
Copy link
Member

ibuziuk commented Sep 25, 2019

ci-build

@ibuziuk ibuziuk merged commit 053b2fd into eclipse-che:master Sep 25, 2019
@tomgeorge tomgeorge deleted the che-14155 branch September 25, 2019 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement A feature request - must adhere to the feature request template. status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants