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

Add chart displaying available applications #286

Merged
merged 14 commits into from
May 29, 2024

Conversation

cbartz
Copy link
Collaborator

@cbartz cbartz commented May 23, 2024

Applicable spec: n/a

Overview

  1. Rename flavour to application in the dashboard panels.
  2. Add a chart close to the input variable listing all applications.

For the short-term board, list all available runners (idle runners after last reconciliation).

image

For the long-term board, show all runners created per application.
image

Rationale

  1. The term flavour is nowhere well defined, and we are really referring to the charm application that manages the runner instances.
  2. The panel should help to list all available applications which can be filtered.

Juju Events Changes

n/a

Module Changes

n/a

Library Changes

n/a

Checklist

@cbartz cbartz added the trivial label May 23, 2024
@cbartz cbartz requested a review from a team as a code owner May 23, 2024 12:02
Copy link
Collaborator

@arturo-seijas arturo-seijas left a comment

Choose a reason for hiding this comment

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

I think the jobs per application metric would be better displayed as a bar chart. The current chart doesn't seem to intuitive to me

@cbartz
Copy link
Collaborator Author

cbartz commented May 23, 2024

@arturo-seijas

I think the jobs per application metric would be better displayed as a bar chart. The current chart doesn't seem to intuitive to me

The problem with bar charts is that the focus is on the graph and not on the legend. The idea here is to present a list of available applications, so the focus should be on the text. My first version was as follows:

image

@arturo-seijas
Copy link
Collaborator

@arturo-seijas

I think the jobs per application metric would be better displayed as a bar chart. The current chart doesn't seem to intuitive to me

The problem with bar charts is that the focus is on the graph and not on the legend. The idea here is to present a list of available applications, so the focus should be on the text. My first version was as follows:

image

What about an horizontal bar chart, with the legend on the left side?

@cbartz
Copy link
Collaborator Author

cbartz commented May 23, 2024

What about an horizontal bar chart, with the legend on the left side?

@arturo-seijas This would look like

image

But as mentioned the emphasis would be a bit fewer on the application names.

There is also already a panel at the bottom of the dashboard that shows the application share and looks like this:

image

This could simply be reused and moved to the top to indicate the list of applications.

Perhaps @jdkandersson can give his opinion on this, as the new panel was requested by him.

@arturo-seijas
Copy link
Collaborator

What about an horizontal bar chart, with the legend on the left side?

@arturo-seijas This would look like

image

But as mentioned the emphasis would be a bit fewer on the application names.

There is also already a panel at the bottom of the dashboard that shows the application share and looks like this:

image

This could simply be reused and moved to the top to indicate the list of applications.

Perhaps @jdkandersson can give his opinion on this, as the new panel was requested by him.

When I see the application share I find too difficult to see the values for those that are not at the top nor bottom. In the screenshot above, what are the values for large? Again, I think bar charts are in order here, something like this

@cbartz
Copy link
Collaborator Author

cbartz commented May 23, 2024

When I see the application share I find too difficult to see the values for those that are not at the top nor bottom. In the screenshot above, what are the values for large? Again, I think bar charts are in order here, something like this

@arturo-seijas Ok, gotcha. I'm fine with using bar charts, but I think I need to re-confirm that this meets the acceptance criteria, as the original story was just to visualise the names of applications, not the number of jobs or anything. I just added it because the log panel only showing the names didn't look "pretty" to me. See also the jira ticket.

@varshigupta12
Copy link

@cbartz I am wondering if all we need is an easy way for users to search by application- can we change the Flavors textbox to be a dropdown instead? Something like-

image

@cbartz
Copy link
Collaborator Author

cbartz commented May 24, 2024

@cbartz I am wondering if all we need is an easy way for users to search by application- can we change the Flavors textbox to be a dropdown instead? Something like-

image

@varshigupta12 The text box is automatically generated by Grafana and is not intended for further customisation. There are drop-down menus for "Query variables", but unfortunately these are not applicable here because the "flavor" is not a real label. It might be possible to turn it into a real label, which might require some customisation of the grafana-agent charm. I think we (@jdkandersson and I) decided not to put too much effort into solving this problem, but it could certainly be investigated together with the observability team.

@jdkandersson
Copy link
Contributor

jdkandersson commented May 27, 2024

@cbartz I am wondering if all we need is an easy way for users to search by application- can we change the Flavors textbox to be a dropdown instead? Something like-
image

@varshigupta12 The text box is automatically generated by Grafana and is not intended for further customisation. There are drop-down menus for "Query variables", but unfortunately these are not applicable here because the "flavor" is not a real label. It might be possible to turn it into a real label, which might require some customisation of the grafana-agent charm. I think we (@jdkandersson and I) decided not to put too much effort into solving this problem, but it could certainly be investigated together with the observability team.

Yeh we couldn't find a way to do a drop down and this is the alternative.

Perhaps we do the horizontal bar chart and make the number the number of available runners? I think that would be a useful chart anyway

arturo-seijas
arturo-seijas previously approved these changes May 28, 2024
@cbartz
Copy link
Collaborator Author

cbartz commented May 28, 2024

@jdkandersson Can you please review? Thanks.

Copy link
Contributor

Test coverage for aeebd7b

Name                                       Stmts   Miss Branch BrPart  Cover   Missing
--------------------------------------------------------------------------------------
src/charm.py                                 462     88    105     22    78%   113-114, 116-117, 122-123, 209-211, 264-283, 301-303, 304->308, 334-338, 422-451, 487-492, 509, 530, 542-548, 562-563, 576, 608-609, 612->617, 651, 655-660, 696, 705->708, 726-727, 759-772, 801-806, 820, 867-868, 870-871, 873-874, 948->950, 1015-1016, 1054-1056, 1073
src/charm_state.py                           346      0     76      0   100%
src/errors.py                                 41      0      0      0   100%
src/event_timer.py                            54      7      2      1    86%   105-106, 131, 148-149, 165-166
src/firewall.py                               51     18     20      0    61%   42-43, 66-69, 111-185
src/github_client.py                          95     16     40      5    79%   65-72, 119->exit, 124-125, 195, 218, 231-238, 260->295, 289
src/github_metrics.py                         13      0      0      0   100%
src/github_type.py                            49      0      0      0   100%
src/lxd_type.py                               35      0      2      0   100%
src/metrics.py                                75      2     10      1    96%   62->65, 172-173
src/metrics_type.py                            5      0      0      0   100%
src/openstack_cloud/__init__.py               26      0      2      0   100%
src/openstack_cloud/openstack_manager.py     126     16     26      1    86%   108, 376-379, 427, 456-482
src/runner.py                                335     74     96     24    73%   46->48, 192->206, 264-265, 291-292, 336-345, 369-374, 379, 403, 407-417, 466->469, 472-474, 481, 495, 510, 514, 516, 533, 567-572, 591-604, 616-655, 664, 707, 716-717, 764-766, 770, 791, 830, 858, 863-875, 893, 898->900, 903-905
src/runner_logs.py                            34      2      6      1    92%   62->61, 66-67
src/runner_manager.py                        320     59    108     13    81%   127, 172-174, 187-188, 200-202, 208-213, 217-218, 253, 296-297, 341-343, 417, 427, 456, 467-471, 496, 519-522, 588-606, 623-628, 652-654, 672-673, 786, 808-810
src/runner_manager_type.py                    38      0      6      0   100%
src/runner_metrics.py                        121      5     18      0    96%   215-216, 331-335
src/runner_type.py                            31      0      8      0   100%
src/shared_fs.py                             125     18     20      0    88%   106-107, 131-132, 215-216, 246-247, 259-260, 286-287, 293-294, 318-319, 326-327
src/utilities.py                              67      7     20      7    82%   85->87, 89->95, 102, 132, 146, 185-188, 243
--------------------------------------------------------------------------------------
TOTAL                                       2449    312    565     75    85%

Static code analysis report

Run started:2024-05-28 07:53:50.356924

Test results:
  No issues identified.

Code scanned:
  Total lines of code: 5839
  Total lines skipped (#nosec): 1
  Total potential issues skipped due to specifically being disabled (e.g., #nosec BXXX): 7

Run metrics:
  Total issues (by severity):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
  Total issues (by confidence):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
Files skipped (0):

@cbartz cbartz merged commit c90b514 into main May 29, 2024
61 checks passed
@cbartz cbartz deleted the feat/add-chart-available-flavors-ISD-1845 branch May 29, 2024 08:15
yhaliaw pushed a commit that referenced this pull request Jun 26, 2024
* Add panel for available applications

* Rename flavor -> application

* Rename flavor -> application

* pin requests < 2.32.0

* flavours -> applications

* use bar gauge

* use bar gauge

* update docs

* Revert "pin requests < 2.32.0"

This reverts commit 767bb62.

* use barchart

* change to available runners

* update docs

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

Successfully merging this pull request may close these issues.

None yet

4 participants