feat(ui): added notification rule & checks last run status & error #16338

asalem1 · 2019-12-27T17:34:43Z

Closes #15838

Solution

Added lastrunstatus check and error handling for notification rules and check rules

CHANGELOG.md updated with a link to the PR (not the Issue)

…ck rules

…with program

kelwang

the go part LGTM

asalem1 · 2019-12-27T17:39:17Z

ui/src/alerting/actions/checks.ts

@@ -182,6 +182,18 @@ export const saveCheckFromTimeMachine = () => async (
  }
 }

+export const updateCheck = (check: Partial<Check>) => async (


this was previously removed and caused a breaking change. It has been added back here with the intention of refactoring this implementation with a different ticket

does this still cause a breaking change? can you link the relevant tickets please?

Yes, when I had originally submitted the comment I hadn't had the chance to write up the issue. Here's the breaking change PR:

#16333
but it seems like the changes made to updateCheck weren’t updated here:
https://github.com/influxdata/influxdb/blob/master/ui/src/alerting/components/CheckCard.tsx#L17

and here's the issue that is intended on resolving it :

#16339

asalem1 · 2019-12-27T17:39:35Z

ui/src/alerting/components/CheckCard.tsx

        <>{relativeTimestampFormatter(check.updatedAt, 'Last updated ')}</>,
+        <LastRunTaskStatus
+          key={2}


added the key here to resolve linter errors

is this a react warning or a lint error? if react is generating a warning then that's the issue we should address. if lint is mistakenly saying we need a key, we should comment an override here, because it's adding meaning to the code to get around a tool issue. do you understand the difference? a real react warning about a list item not having a key is something we should fix, a mistaken lint error trying to head off that warning is something we should comment about, but not change our code to make go away.

if it's an actual react warning, we should give this a more unique name, if possible. react says to only use the item's list index as a last resort

When you don’t have stable IDs for rendered items, you may use the item index as a key as a last resort:

We don’t recommend using indexes for keys if the order of items may change. This can negatively impact performance and may cause issues with component state.

https://reactjs.org/docs/lists-and-keys.html#keys

same comment and any changes that need to happen apply to RuleCard's key change

It was a lint error - i imagine stemming from the way clockface handles metaData being passed dom nodes

asalem1 · 2019-12-27T17:39:47Z

ui/src/alerting/components/notifications/RuleCard.tsx

        <>{relativeTimestampFormatter(rule.updatedAt, 'Last updated ')}</>,
+        <LastRunTaskStatus
+          key={2}


added key prop here to resolve linter errors

hoorayimhelping

Some comments that appear to be too late.

hoorayimhelping · 2019-12-27T17:40:53Z

http/check_test.go

@@ -1130,7 +1140,9 @@ func TestService_handleUpdateCheck(t *testing.T) {
          "statusMessageTemplate": "",
          "tags": null,
          "type": "deadman",
-		  "labels": []
+					"labels": [],


looks like there's some spacing issue here?

hoorayimhelping · 2019-12-27T17:43:47Z

http/notification_rule_test.go

@@ -147,7 +147,9 @@ func Test_newNotificationRuleResponses(t *testing.T) {
      ],
      "type": "slack",
      "updatedAt": "0001-01-01T00:00:00Z",
-      "status": "active"
+      "status": "active",
+			"latestCompleted": "0001-01-01T00:00:00Z",


@hoorayimhelping go won't allow for weird formatting. This may look strange but it's kosher in the actual files

i know gfmt controls spacing, but those things can get weird sometimes, it's always good to confirm and not just trust the machines

for sure. It's all good

hoorayimhelping · 2019-12-27T17:43:54Z

http/notification_rule_test.go

- "updatedAt": "0001-01-01T00:00:00Z"
+ "updatedAt": "0001-01-01T00:00:00Z",
+ "latestCompleted": "0001-01-01T00:00:00Z",
+	"latestScheduled": "0001-01-01T00:00:00Z"


same as above

hoorayimhelping · 2019-12-27T17:54:12Z

ui/src/alerting/actions/checks.ts

@@ -182,6 +182,18 @@ export const saveCheckFromTimeMachine = () => async (
  }
 }

+export const updateCheck = (check: Partial<Check>) => async (


does this still cause a breaking change? can you link the relevant tickets please?

hoorayimhelping · 2019-12-27T18:24:42Z

ui/src/alerting/components/CheckCard.tsx

        <>{relativeTimestampFormatter(check.updatedAt, 'Last updated ')}</>,
+        <LastRunTaskStatus
+          key={2}


is this a react warning or a lint error? if react is generating a warning then that's the issue we should address. if lint is mistakenly saying we need a key, we should comment an override here, because it's adding meaning to the code to get around a tool issue. do you understand the difference? a real react warning about a list item not having a key is something we should fix, a mistaken lint error trying to head off that warning is something we should comment about, but not change our code to make go away.

if it's an actual react warning, we should give this a more unique name, if possible. react says to only use the item's list index as a last resort

When you don’t have stable IDs for rendered items, you may use the item index as a key as a last resort:

We don’t recommend using indexes for keys if the order of items may change. This can negatively impact performance and may cause issues with component state.

https://reactjs.org/docs/lists-and-keys.html#keys

same comment and any changes that need to happen apply to RuleCard's key change

hoorayimhelping · 2019-12-27T18:26:57Z

ui/src/shared/components/lastRunTaskStatus/LastRunTaskStatus.tsx

+} from '@influxdata/clockface'
+
+// Styles
+import './LastRunTaskStatus.scss'


is this where we want to import this? or should it be imported in in chronograf.scss ?

@hoorayimhelping this was input by @alexpaxton. I'm not entirely sure on best practices but if it's the case that it should be input into the chronograf file I can go ahead and do that

hoorayimhelping · 2019-12-27T18:31:04Z

ui/src/shared/components/lastRunTaskStatus/LastRunTaskStatus.tsx

+  const triggerRef = useRef<HTMLDivElement>(null)
+  const [highlight, setHighlight] = useState<boolean>(false)
+
+  let color = ComponentColor.Success


are there implications to setting the default state to success? if a task run fails and we don't know about it, and we set the status to success, there might be a nonzero impact on the user? what kind of thought have we put into that?

If a task run fails and we don't know about it, that's a failure on the API

hoorayimhelping · 2019-12-27T18:33:55Z

ui/src/shared/components/lastRunTaskStatus/LastRunTaskStatus.tsx

+  let icon = IconFont.Checkmark
+  let text = 'Task ran successfully!'
+
+  if (lastRunStatus === 'failed' || lastRunError !== undefined) {


lastRunError !== undefined

is this this check we want here (rather than a more greedy falsy check)? lastRunError = false | null | '' | {} will pass this check and make the app enter an error state. is that desirable?

That's a great point and definitely something to consider. My thinking here was that since the lastRunError is an optional parameter (it can be undefined), and since the lastRunError should be a string if it is defined, the check for explicitly not undefined (meaning a string should exist) should be sufficient. Anything besides a string should trigger an error for us since our API would be behaving unexpectedly if it did return a more generally false check (like false null). In this case, an empty string being passed in would be more indicative of improper error handling, but not a reason to prevent the functionality

…16338) feat(ui): added last run status checks for notification rules and check rules, readded updateCheck to fix linter and functionality issues with program and added tests to ensure check creation and update stability

asalem1 added 4 commits December 27, 2019 06:22

feat(ui): added last run status checks for notification rules and che…

4bfbf1e

…ck rules

feat(ui): readded updateCheck to fix linter and functionality issues …

dd6c809

…with program

feat(ui): added tests to ensure check creation and update stability

c96a64f

fix(ui): removed only from test

d2d06f5

asalem1 requested review from a team December 27, 2019 17:34

asalem1 requested a review from a team as a code owner December 27, 2019 17:34

ghost requested review from 121watts and hoorayimhelping and removed request for a team December 27, 2019 17:34

kelwang approved these changes Dec 27, 2019

View reviewed changes

chore(ui): added changelog changes

0432784

asalem1 commented Dec 27, 2019

View reviewed changes

asalem1 merged commit 79c5c79 into master Dec 27, 2019

hoorayimhelping reviewed Dec 27, 2019

View reviewed changes

asalem1 deleted the notification-rules branch December 27, 2019 22:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ui): added notification rule & checks last run status & error #16338

feat(ui): added notification rule & checks last run status & error #16338

asalem1 commented Dec 27, 2019

kelwang left a comment

asalem1 Dec 27, 2019

hoorayimhelping Dec 27, 2019

asalem1 Dec 27, 2019

asalem1 Dec 27, 2019

hoorayimhelping Dec 27, 2019

asalem1 Dec 27, 2019

asalem1 Dec 27, 2019

hoorayimhelping left a comment •

edited

Loading

hoorayimhelping Dec 27, 2019

hoorayimhelping Dec 27, 2019

asalem1 Dec 27, 2019

hoorayimhelping Dec 27, 2019

asalem1 Dec 27, 2019

hoorayimhelping Dec 27, 2019

asalem1 Dec 27, 2019

hoorayimhelping Dec 27, 2019

hoorayimhelping Dec 27, 2019

hoorayimhelping Dec 27, 2019

asalem1 Dec 27, 2019

hoorayimhelping Dec 27, 2019

asalem1 Dec 27, 2019

hoorayimhelping Dec 27, 2019

asalem1 Dec 27, 2019

feat(ui): added notification rule & checks last run status & error #16338

feat(ui): added notification rule & checks last run status & error #16338

Conversation

asalem1 commented Dec 27, 2019

Solution

kelwang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoorayimhelping left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoorayimhelping left a comment •

edited

Loading