Bug 1868013: Point admin monitoring links to admin #6263

abhinandan13jan · 2020-08-07T13:36:15Z

Adresses

https://issues.redhat.com/browse/ODC-4397

Issue

Monitoring links in admin point to Dev-monitoring. Use of getCurrentPerspective poses problem in incognito

Solution

Altering Links to conditionally point to admin/dev monitoring based on the current URL(location)

Screenshot

Test

Fixed

Browser

Chrome

debsmita1 · 2020-08-11T10:20:02Z

Thank you for working on this. This works fine, I'm just unsure about the part where you are redirecting the user to /monitoring/dashboards when clicked on view monitoring dashboard in admin perspective. In the dev side, we do show workload specific dashboard so that part is fine. May be @cshinn can help here

abhinandan13jan · 2020-08-11T12:10:11Z

/retitle Bug 1868013: Point admin monitoring links to admin

openshift-ci-robot · 2020-08-11T12:10:31Z

@abhinandan13jan: This pull request references Bugzilla bug 1868013, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1868013: Point admin monitoring links to admin

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

abhinandan13jan · 2020-08-11T12:11:22Z

/kind bug

sahil143 · 2020-08-11T17:26:12Z

frontend/packages/dev-console/src/components/monitoring/overview/MonitoringOverview.tsx

@@ -132,7 +133,11 @@ const MonitoringOverview: React.FC<MonitoringOverviewProps> = (props) => {
              <>
                <div className="odc-monitoring-overview__view-monitoring-dashboard">
                  <Link
-                    to={`/dev-monitoring/ns/${resource?.metadata?.namespace}/?workloadName=${resource?.metadata?.name}&workloadType=${resource?.kind}`}
+                    to={
+                      currentPath.startsWith('/k8s')


I think it would be better if we can do it based on the active perspective. WDYT?

@sahil143 The issue with checking last active perspective is that it returns 'admin' on incognito while You are on dev

@abhinandan13jan Why would last active perspective return admin on incognito while you're on dev? Both localStorage and redux store gets initialized as expected on incognito as well otherwise the application wouldn't work.

I don't think it's a good idea to check for a specific URL pattern. What if this component is added to some other page later which doesn't conform to the above URL pattern? The whole logic would break then. I think it's better to use activePerspective from redux here..

I agree with @rohitkrai03. We shouldn't be checking based on the URL pattern. If it is returning admin on incognito while in dev then it's a bug and we should look to fix it.

@sahil143 @rohitkrai03 The problem is if you copy a dev-perspective URL and open it in incognito, the activePerspective in the redux is set as admin . Have a look at the below screenshot

@debsmita1 That happens for any devconsole url in incognito. I think it's a known issue.

I have updated to use getActivePerspective for convention as well and it seems to work fine given that the whole thing would finally run at a cluster URl

cshinn · 2020-08-12T13:52:36Z

@abhinandan13jan @debsmita1 It seems like the best thing to link to in the admin console would be the grafana-dashboard-k8s-resources-workload board for the relevant namespace/workload. It has the same context as the dev version would and shows the same data as the sidebar

abhinandan13jan · 2020-08-17T09:21:05Z

@abhinandan13jan @debsmita1 It seems like the best thing to link to in the admin console would be the grafana-dashboard-k8s-resources-workload board for the relevant namespace/workload. It has the same context as the dev version would and shows the same data as the sidebar

@cshinn We can redirect to monitoring/dashboards/grafana-dashboard-k8s-resources-workload but the namespace or workload name cannot be sent over the URL. So, it will not have the context cc: @debsmita1

I've updated to use grafana-dashboard-k8s-resources-workload dashboard

openshift-ci-robot · 2020-08-18T07:17:32Z

@abhinandan13jan: This pull request references Bugzilla bug 1868013, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1868013: Point admin monitoring links to admin

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

debsmita1 · 2020-08-18T08:22:43Z

cc @invincibleJai @kyoto ^

rohitkrai03 · 2020-08-18T16:48:53Z

frontend/packages/dev-console/src/components/monitoring/overview/MonitoringOverview.tsx

@@ -132,7 +133,11 @@ const MonitoringOverview: React.FC<MonitoringOverviewProps> = (props) => {
              <>
                <div className="odc-monitoring-overview__view-monitoring-dashboard">
                  <Link
-                    to={`/dev-monitoring/ns/${resource?.metadata?.namespace}/?workloadName=${resource?.metadata?.name}&workloadType=${resource?.kind}`}
+                    to={
+                      currentPath.startsWith('/k8s')


@abhinandan13jan Why would last active perspective return admin on incognito while you're on dev? Both localStorage and redux store gets initialized as expected on incognito as well otherwise the application wouldn't work.

rohitkrai03 · 2020-08-18T16:49:05Z

frontend/packages/dev-console/src/components/monitoring/overview/MonitoringOverview.tsx

@@ -132,7 +133,11 @@ const MonitoringOverview: React.FC<MonitoringOverviewProps> = (props) => {
              <>
                <div className="odc-monitoring-overview__view-monitoring-dashboard">
                  <Link
-                    to={`/dev-monitoring/ns/${resource?.metadata?.namespace}/?workloadName=${resource?.metadata?.name}&workloadType=${resource?.kind}`}
+                    to={
+                      currentPath.startsWith('/k8s')


I don't think it's a good idea to check for a specific URL pattern. What if this component is added to some other page later which doesn't conform to the above URL pattern? The whole logic would break then. I think it's better to use activePerspective from redux here..

abhinandan13jan · 2020-08-19T12:20:41Z

@rohitkrai03 @sahil143 @debsmita1 I have updated to use getActivePerspective and it seems to work fine

abhinandan13jan · 2020-08-19T12:59:10Z

/retest

rohitkrai03

/lgtm

openshift-bot · 2020-08-20T06:34:41Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-20T08:44:42Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-20T08:57:40Z

/retest

Please review the full test history for this PR and help us cut down flakes.

invincibleJai · 2020-08-20T10:58:54Z

/hold

invincibleJai · 2020-08-20T11:01:38Z

in Admin perspective if i go from sidebar to dashboard , it doesn't selects current namespace

@cshinn We can redirect to monitoring/dashboards/grafana-dashboard-k8s-resources-workload but the namespace or workload name cannot be sent over the URL. So, it will not have the context cc: @debsmita1

@kyoto can add here , if there is a way to update proper ns/workloads in this scenario or redirecting to grafana-dashboard-k8s-resources-workload is the way now cc @abhinandan13jan

rohitkrai03 · 2020-08-20T11:46:01Z

in Admin perspective if i go from sidebar to dashboard , it doesn't selects current namespace

@cshinn We can redirect to monitoring/dashboards/grafana-dashboard-k8s-resources-workload but the namespace or workload name cannot be sent over the URL. So, it will not have the context cc: @debsmita1

@kyoto can add here , if there is a way to update proper ns/workloads in this scenario or redirecting to grafana-dashboard-k8s-resources-workload is the way now cc @abhinandan13jan

AFAIK, there's no support for passing namespace or workload context in the grafana dashboard route currently. Even the namespace dropdown selects first item in the dropdown instead of activeNamespace from redux store. This results in user being redirected to wrong namespace context. Not sure what we can do for the scope of this PR.

In 4.5 we are sending user to dev monitoring dashboard which keeps user in correct namespace context. Maybe we should keep doing that till we have support for namespace and workload context in grafana dashboard on admin side. WDYT @kyoto?
cc: @christianvogt

kyoto · 2020-08-21T06:44:00Z

In the admin perspective Dashboards page, the "Namespace" dropdown represents a variable in the dashboard definition (same as "Workload" and "Type"). The page therefore doesn't have any knowledge of what the variable "Namespace" means, so it isn't connected to the active namespace.

Unfortunately, there's currently no way to automatically select a value from the "Namespace" dropdown (or any other variable dropdown).

abhinandan13jan · 2020-08-21T10:13:37Z

/retest

invincibleJai · 2020-08-21T13:37:24Z

In the admin perspective Dashboards page, the "Namespace" dropdown represents a variable in the dashboard definition (same as "Workload" and "Type"). The page therefore doesn't have any knowledge of what the variable "Namespace" means, so it isn't connected to the active namespace.

Unfortunately, there's currently no way to automatically select a value from the "Namespace" dropdown (or any other variable dropdown).

Make sense so existing 4.5 behaviour seems better than linking to the admin perspective dashboard for a different namespace cc @cshinn

invincibleJai · 2020-08-21T14:51:33Z

/hold cancel

invincibleJai · 2020-08-21T15:40:23Z

/lgtm

Verified the changes

openshift-ci-robot · 2020-08-21T15:40:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinandan13jan, invincibleJai, rohitkrai03

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:

~~frontend/packages/dev-console/OWNERS~~ [invincibleJai,rohitkrai03]

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

openshift-bot · 2020-08-21T17:46:39Z

/retest

Please review the full test history for this PR and help us cut down flakes.

andrewballantyne · 2020-08-21T18:04:58Z

/retest

openshift-bot · 2020-08-21T20:08:51Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2020-08-22T00:52:20Z

@abhinandan13jan: All pull requests linked via external trackers have merged: openshift/console#6263. Bugzilla bug 1868013 has been moved to the MODIFIED state.

In response to this:

Bug 1868013: Point admin monitoring links to admin

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. component/dev-console Related to dev-console labels Aug 7, 2020

openshift-ci-robot requested review from andrewballantyne and invincibleJai August 7, 2020 13:36

abhinandan13jan force-pushed the monitoring-links branch 2 times, most recently from 606387c to a613e50 Compare August 11, 2020 08:26

abhinandan13jan changed the title ~~WIP Monitoring links~~ Point admin monitoring links to admin Aug 11, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 11, 2020

openshift-ci-robot changed the title ~~Point admin monitoring links to admin~~ Bug 1868013: Point admin monitoring links to admin Aug 11, 2020

openshift-ci-robot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Aug 11, 2020

openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Aug 11, 2020

sahil143 reviewed Aug 11, 2020

View reviewed changes

abhinandan13jan force-pushed the monitoring-links branch from a613e50 to 24420e1 Compare August 17, 2020 09:48

rohitkrai03 suggested changes Aug 18, 2020

View reviewed changes

abhinandan13jan force-pushed the monitoring-links branch from 24420e1 to 942696a Compare August 19, 2020 12:18

rohitkrai03 approved these changes Aug 20, 2020

View reviewed changes

openshift-ci-robot assigned rohitkrai03 Aug 20, 2020

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 20, 2020

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 20, 2020

abhinandan13jan force-pushed the monitoring-links branch from 942696a to 2d1ab62 Compare August 21, 2020 09:45

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 21, 2020

admin monitoring links should point to admin

cfd1d17

abhinandan13jan force-pushed the monitoring-links branch from 2d1ab62 to cfd1d17 Compare August 21, 2020 10:21

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 21, 2020

openshift-ci-robot assigned invincibleJai Aug 21, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 21, 2020

openshift-merge-robot merged commit 11fe709 into openshift:master Aug 22, 2020

spadgett added this to the v4.6 milestone Aug 26, 2020

Bug 1868013: Point admin monitoring links to admin #6263

Bug 1868013: Point admin monitoring links to admin #6263

Conversation

abhinandan13jan commented Aug 7, 2020 • edited

Adresses

Issue

Solution

Screenshot

Test

Browser

debsmita1 commented Aug 11, 2020

abhinandan13jan commented Aug 11, 2020

openshift-ci-robot commented Aug 11, 2020

abhinandan13jan commented Aug 11, 2020

sahil143 Aug 11, 2020

Choose a reason for hiding this comment

abhinandan13jan Aug 17, 2020

Choose a reason for hiding this comment

rohitkrai03 Aug 18, 2020

Choose a reason for hiding this comment

rohitkrai03 Aug 18, 2020

Choose a reason for hiding this comment

sahil143 Aug 18, 2020

Choose a reason for hiding this comment

debsmita1 Aug 18, 2020 • edited

Choose a reason for hiding this comment

sahil143 Aug 19, 2020 • edited

Choose a reason for hiding this comment

abhinandan13jan Aug 19, 2020 • edited

Choose a reason for hiding this comment

cshinn commented Aug 12, 2020 • edited

abhinandan13jan commented Aug 17, 2020 • edited

openshift-ci-robot commented Aug 18, 2020

debsmita1 commented Aug 18, 2020

rohitkrai03 Aug 18, 2020

Choose a reason for hiding this comment

rohitkrai03 Aug 18, 2020

Choose a reason for hiding this comment

abhinandan13jan commented Aug 19, 2020

abhinandan13jan commented Aug 19, 2020

rohitkrai03 left a comment

Choose a reason for hiding this comment

openshift-bot commented Aug 20, 2020

openshift-bot commented Aug 20, 2020

openshift-bot commented Aug 20, 2020

invincibleJai commented Aug 20, 2020

invincibleJai commented Aug 20, 2020 • edited

rohitkrai03 commented Aug 20, 2020 • edited

kyoto commented Aug 21, 2020

abhinandan13jan commented Aug 21, 2020

invincibleJai commented Aug 21, 2020

invincibleJai commented Aug 21, 2020

invincibleJai commented Aug 21, 2020

openshift-ci-robot commented Aug 21, 2020

openshift-bot commented Aug 21, 2020

andrewballantyne commented Aug 21, 2020

openshift-bot commented Aug 21, 2020

openshift-ci-robot commented Aug 22, 2020

abhinandan13jan commented Aug 7, 2020 •

edited

debsmita1 Aug 18, 2020 •

edited

sahil143 Aug 19, 2020 •

edited

abhinandan13jan Aug 19, 2020 •

edited

cshinn commented Aug 12, 2020 •

edited

abhinandan13jan commented Aug 17, 2020 •

edited

invincibleJai commented Aug 20, 2020 •

edited

rohitkrai03 commented Aug 20, 2020 •

edited