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/update queries for pulling Key Usage Indicators data #149

Merged
merged 19 commits into from
Mar 28, 2024
Merged

Add/update queries for pulling Key Usage Indicators data #149

merged 19 commits into from
Mar 28, 2024

Conversation

afgane
Copy link
Contributor

@afgane afgane commented Mar 25, 2024

Requested by @bgruening to start including EU data on the Key Usage Indicators dashboard, ported necessary queries to gxadmin.

toolshed.g2.bx.psu.edu/repos/rnateam/graphclust_nspdk/nspdk_sparse/9.2 | 52861
Filter1 | 43253
$ gxadmin tool-usage-over-time
tool_id | count
Copy link
Member

Choose a reason for hiding this comment

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

Looks like the formatter has caused a misalignment of columns

Copy link
Member

Choose a reason for hiding this comment

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

Yes. The 4 spacing is intentional here, it generates a code block without being fenced which is more difficult to parse in bash. Could you please revert these spacing changes?

Copy link
Member

@hexylena hexylena left a comment

Choose a reason for hiding this comment

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

I'll review again once the whitespace issue is fixed since it's making the diff harder to read in places.

Looking forward to this change!

I guess you should also consider coordinating with usegalaxy.fr (which meets the usegalaxy.* criteria) to see if they wants to submit aggregated user statistics to this central dashboard.

toolshed.g2.bx.psu.edu/repos/rnateam/graphclust_nspdk/nspdk_sparse/9.2 | 52861
Filter1 | 43253
$ gxadmin tool-usage-over-time
tool_id | count
Copy link
Member

Choose a reason for hiding this comment

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

Yes. The 4 spacing is intentional here, it generates a code block without being fenced which is more difficult to parse in bash. Could you please revert these spacing changes?

parts/22-query.sh Show resolved Hide resolved
parts/22-query.sh Outdated Show resolved Hide resolved
parts/22-query.sh Outdated Show resolved Hide resolved
parts/22-query.sh Outdated Show resolved Hide resolved
@afgane
Copy link
Contributor Author

afgane commented Mar 26, 2024

I reverted the big tabs change; sorry about that. It should be easier to review now.

Thanks for the initial comments.

And yes, .fr as well as any other .* instance will be more than welcome to contribute the data. Starting with the big three as exemplars, plus getting the queries into here, will hopefully make that more palatable for other servers.

Copy link
Member

@hexylena hexylena left a comment

Choose a reason for hiding this comment

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

This looks great, thank you for the KPI queries, I think this is a great batch of changes, I have a small comment on standardisation of the CLI across the commands but I think this is very close to done.

Thanks for filling out the changelog as well :)

@@ -2286,52 +2286,63 @@ query_group-gpu-time() { ##? [group]: Retrieve an approximation of the GPU time
EOF
}

query_monthly-users-registered(){ ## [year] [--by_group]: Number of users registered each month
query_monthly-users-registered(){ ## [year] [YYYY-MM] [--by_group]: Number of users registered
Copy link
Member

Choose a reason for hiding this comment

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

so this one is a bit odd for me, with year + yyyy-mm support. These arguments effectively act as filter, and I think we can simplify them, and make them match up a bit better with other commands like query_monthly-job-runtimes

Suggested change
query_monthly-users-registered(){ ## [year] [YYYY-MM] [--by_group]: Number of users registered
query_monthly-users-registered(){ ## [--year=<YYYY>] [--month=<MM>] [--by_group]: Number of users registered

I think this could then be copied and pasted across the rest and we can remove the (lovely!!) use of regexes for matching strings and have a nicer more consistent UX.

parts/22-query.sh Outdated Show resolved Hide resolved
@@ -2344,7 +2355,7 @@ query_monthly-users-registered(){ ## [year] [--by_group]: Number of users regist
EOF
}

query_monthly-users-active(){ ## [year] [--by_group]: Number of active users per month, running jobs
query_monthly-users-active(){ ## [year] [YYYY-MM] [--by_group]: Number of active users per month, running jobs
Copy link
Member

Choose a reason for hiding this comment

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

same here

@afgane
Copy link
Contributor Author

afgane commented Mar 27, 2024

Thanks for that suggestion. I also didn't like how that looked but didn't want to change method signatures. This is now much better in terms of the UX as well as cleaner code.

Copy link
Member

@hexylena hexylena left a comment

Choose a reason for hiding this comment

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

Yes, fantastic, I love it!

@hexylena hexylena merged commit 9f90b37 into main Mar 28, 2024
1 check passed
@afgane afgane deleted the kui branch March 28, 2024 13:58
@afgane
Copy link
Contributor Author

afgane commented Mar 28, 2024

Thanks for the quick merge!

@hexylena
Copy link
Member

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

Successfully merging this pull request may close these issues.

3 participants