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.

Sign up for GitHub

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

Jump to bottom

Node metrics integrations #290

Merged
merged 10 commits into from
Aug 21, 2023
Merged

Node metrics integrations #290

merged 10 commits into from
Aug 21, 2023

Conversation

torchiaf
Copy link
Contributor

@torchiaf torchiaf commented Aug 3, 2023 •
edited
Loading

Summary

Fixes #270

Screenshots

image

@torchiaf torchiaf self-assigned this Aug 11, 2023
@torchiaf torchiaf marked this pull request as ready for review August 11, 2023 14:23
@torchiaf
Add warning in case of low node resources; fetching Node Metrics
cd059b6 
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
@torchiaf torchiaf changed the title Show node metrics Node metrics integrations Aug 11, 2023
richard-cox
richard-cox requested changes Aug 14, 2023
View reviewed changes
Copy link
Member

@richard-cox richard-cox left a comment

Choose a reason for hiding this comment

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

I'm not sure we've got the UX right for this. Users might be ok with 5% of resources left and the warning then becomes an annoyance. Can we change it to a standard notification banner that's always shown (if embedded)?

dashboard/pkg/epinio/pages/c/_cluster/dashboard.vue Outdated Show resolved Hide resolved
dashboard/pkg/epinio/pages/c/_cluster/dashboard.vue Outdated
openMetricsDetails() {
const clusterMetricsLink = `${ window.location.origin }/c/${ this.$store.getters['clusterId'] }/explorer#cluster-events`;

window.open(clusterMetricsLink, '_blank');
Copy link
Member

Choose a reason for hiding this comment

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

this should use $router. also as we're in the dashboard context i think it's find to open in the same tab

Copy link
Member

Choose a reason for hiding this comment

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

this one is still remaining to do

dashboard/pkg/epinio/l10n/en-us.yaml Outdated Show resolved Hide resolved
dashboard/pkg/epinio/pages/c/_cluster/dashboard.vue Outdated Show resolved Hide resolved
dashboard/pkg/epinio/store/epinio-store/actions.ts Show resolved Hide resolved
dashboard/pkg/epinio/types.ts Outdated Show resolved Hide resolved
@torchiaf
Use router to navigate to the cluster explorer
314ea36 
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
@torchiaf
Change label of link to cluster metrics
21511a7 
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
@torchiaf
Move warning banner below welcome card
152a8dc 
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
@torchiaf
Fix METRIC import
14f719d 
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
@torchiaf
Change Banner information and layout
e5a731c 
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
@torchiaf
Clean up dashboard/fetch() function
8e997db 
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
@torchiaf
Copy link
Contributor Author

torchiaf commented Aug 17, 2023 •
edited
Loading

@richard-cox thank for the heads up, I should have fixed everything.
This is how it looks like at the end:

image

@torchiaf
Fix lint
99ad1be 
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
richard-cox
richard-cox reviewed Aug 21, 2023
View reviewed changes
dashboard/pkg/epinio/store/epinio-store/actions.ts Outdated Show resolved Hide resolved
dashboard/pkg/epinio/pages/c/_cluster/dashboard.vue Show resolved Hide resolved
dashboard/pkg/epinio/pages/c/_cluster/dashboard.vue Outdated
openMetricsDetails() {
const clusterMetricsLink = `${ window.location.origin }/c/${ this.$store.getters['clusterId'] }/explorer#cluster-events`;

window.open(clusterMetricsLink, '_blank');
Copy link
Member

Choose a reason for hiding this comment

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

this one is still remaining to do

@richard-cox richard-cox added this to the v1.10.0 milestone Aug 21, 2023
@torchiaf
Check schema only if Epinio Ui is embedded
15d4a67 
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
@torchiaf
Copy link
Contributor Author

torchiaf commented Aug 21, 2023 •
edited
Loading

Thanks @richard-cox , correct lat things.
This was already fixed in the previous round:

window.open(clusterMetricsLink, '_blank');

probably an error with your GH view

@richard-cox
Changes from review
ce33365 
- use $router with location
- don't attempt to fetch cluster metrics if in standalone mode
richard-cox
richard-cox approved these changes Aug 21, 2023
View reviewed changes
Copy link
Member

@richard-cox richard-cox left a comment

Choose a reason for hiding this comment

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

I pushed two changes

  • use $router with location
  • don't attempt to fetch cluster metrics if in standalone mode (it'll never work there)

@torchiaf torchiaf merged commit 6e50a1e into epinio:main Aug 21, 2023
3 checks passed
@mmartin24 mmartin24 mentioned this pull request Aug 28, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richard-cox richard-cox richard-cox approved these changes

@scures scures Awaiting requested review from scures

Assignees

@torchiaf torchiaf

Labels
None yet
Projects
None yet
Milestone
v1.10.0
Development

Successfully merging this pull request may close these issues.

[ui] Embedded - Show charts based on Kube metric server resources
2 participants
@torchiaf @richard-cox