Topology: Only retrieve events when showing the monitoring tab #4640

jeff-phillips-18 · 2020-03-04T21:05:31Z

Fixes:
https://issues.redhat.com/browse/ODC-2804

Analysis / Root cause:
Events are being retrieved when loading the topology view and topology is updated when events change. These events are not used unless the details panel is being shown with the monitoring tab selected.

Solution Description:
Do not retrieve events until the user opens the details page and selects the monitoring tab.
Show events from pods and the resource in the events lists.
Sort events chronologically

Screen shots / Gifs for design review:

Unit test coverage report:

Browser conformance:

Chrome
Firefox
Safari
Edge

/kind bug

cc @openshift/team-devconsole-ux

christianvogt · 2020-03-04T22:33:06Z

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

Can we use a field selector to only fetch events related to specific resource.

Looking at events.jsx, this is what they are doing. They also don't use firehose. I'm not familiar enough with the history behind why it's done differently here.

involvedObject.uid=${uid},involvedObject.name=${name},involvedObject.kind=${kind}

Added the selector but stuck with Firehose

invincibleJai · 2020-03-05T15:32:42Z

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

nit: can import import { PodModel } from '@console/internal/models'; and use PodModel.kind

Why is kind equal to pod? shouldn't this be the kind of the object ?

i think it'll be pod as we are fetching events for pods irrespective of workloads i.e d,dc etc

invincibleJai · 2020-03-05T15:33:46Z

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

nit: can be above relative imports

Was this not picked up by the linter?

linter doesn't seem to care anymore

@christianvogt Linter seems to have issues for some time now... I think it was broken when we upgraded everything - It doesn't catch import order I don't think. Might just not catch @ import order.

invincibleJai · 2020-03-05T15:53:38Z

@jeff-phillips-18 monitoring tab is shown now for knative service and revisions as well , am not sure if we need that , underlying they do have deployments and pods

invincibleJai · 2020-03-05T16:10:35Z

frontend/packages/console-shared/src/utils/resource-utils.ts

I think pods above was introduced in #3961 to fetch events for that pod , as we are fetching events now based on selector so we can remove this as well

we use pods in the objects in other places (donut and such). I don't believe they were introduced in that PR on changed to create the pods constant rather than inline.

Yes , got it.

rohitkrai03 · 2020-03-05T18:51:39Z

/retest

rohitkrai03

/lgtm

christianvogt · 2020-03-06T20:33:38Z

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

I think we agreed that the events in the monitoring tab should be the same events shown in the workloads events tab and not the individual pods associated with the workload.

cc @sspeiche @abhi-kn

christianvogt · 2020-03-06T21:00:44Z

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

stray ;

Suggested change

<MonitoringOverviewWrapper item={item} />;

<MonitoringOverviewWrapper item={item} />

invincibleJai · 2020-03-09T12:00:40Z

/lgtm

Verified locally working as expected

can see Monitoring tabs only for workloads and events for associated workloads and not Pods

christianvogt · 2020-03-09T21:38:53Z

/hold
holding until we get agreement on whether to show events related to all pods and the workload, or just the workload itself.

jeff-phillips-18 · 2020-03-11T12:42:37Z

@christianvogt I believe we have come to consensus and we will show events from the pods as well as from the resource.

christianvogt · 2020-03-11T15:36:12Z

/hold cancel

invincibleJai · 2020-03-13T10:24:54Z

/lgtm

christianvogt · 2020-03-13T16:06:57Z

/approve

openshift-ci-robot · 2020-03-13T16:07:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, invincibleJai, jeff-phillips-18, rohitkrai03

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [christianvogt]

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-03-13T16:43:05Z

/retest

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

openshift-bot · 2020-03-13T17:21:59Z

/retest

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

openshift-ci-robot added kind/bug Categorizes issue or PR as related to a bug. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. component/dev-console Related to dev-console component/shared Related to console-shared labels Mar 4, 2020

openshift-ci-robot requested review from andrewballantyne and jhadvig March 4, 2020 21:06

christianvogt reviewed Mar 4, 2020

View reviewed changes

jeff-phillips-18 force-pushed the monitoring-tab branch from d7c27fe to b2445e0 Compare March 5, 2020 12:31

invincibleJai reviewed Mar 5, 2020

View reviewed changes

jeff-phillips-18 force-pushed the monitoring-tab branch from b2445e0 to f9c2df0 Compare March 5, 2020 17:27

rohitkrai03 approved these changes Mar 5, 2020

View reviewed changes

openshift-ci-robot assigned rohitkrai03 Mar 5, 2020

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

christianvogt reviewed Mar 6, 2020

View reviewed changes

jeff-phillips-18 force-pushed the monitoring-tab branch from f9c2df0 to 0f0dcd3 Compare March 7, 2020 15:45

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

openshift-ci-robot assigned invincibleJai Mar 9, 2020

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

Topology: Only retrieve events when showing the monitoring tab

2d21ce7

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

Update to show all Pod events

e46b3c0

jeff-phillips-18 force-pushed the monitoring-tab branch from 0f0dcd3 to e46b3c0 Compare March 11, 2020 12:37

openshift-ci-robot removed lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 11, 2020

openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 11, 2020

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

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

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 13, 2020

openshift-merge-robot merged commit 6905d8d into openshift:master Mar 13, 2020

spadgett added this to the v4.5 milestone Mar 18, 2020

jeff-phillips-18 deleted the monitoring-tab branch December 2, 2020 13:35

	<MonitoringOverviewWrapper item={item} />;
	<MonitoringOverviewWrapper item={item} />

Topology: Only retrieve events when showing the monitoring tab #4640

Topology: Only retrieve events when showing the monitoring tab #4640

Uh oh!

Conversation

jeff-phillips-18 commented Mar 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

invincibleJai commented Mar 5, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rohitkrai03 commented Mar 5, 2020

Uh oh!

rohitkrai03 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

invincibleJai commented Mar 9, 2020

Uh oh!

christianvogt commented Mar 9, 2020

Uh oh!

jeff-phillips-18 commented Mar 11, 2020

Uh oh!

christianvogt commented Mar 11, 2020

Uh oh!

invincibleJai commented Mar 13, 2020

Uh oh!

christianvogt commented Mar 13, 2020

Uh oh!

openshift-ci-robot commented Mar 13, 2020

Uh oh!

openshift-bot commented Mar 13, 2020

Uh oh!

openshift-bot commented Mar 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

jeff-phillips-18 commented Mar 4, 2020 •

edited

Loading