Add target to event existence check #15719

moolitayer · 2017-08-03T12:04:31Z

In Prometheus alerting if in one evaluation cycle, the same alert was first found to be firing
for different labels (==entities) we will get multiple alerts indistinguishable by timestamp[1]

[1]
Example, for both
"startsAt": "2017-08-02T10:16:30.457040707Z"
and there is no other time related difference:

{
  "generationID": "95b45589-ebee-49ca-a1bc-978c401dfde0",
  "messages": [
    {
      "index": 1,
      "timestamp": "2017-08-02T10:17:00.472823555Z",
      "data": {
        "alerts": [
          {
            "annotations": {
              "description": "ocp-compute01.10.35.48.142.nip.io is down_d",
              "message": "ocp-compute01.10.35.48.142.nip.io is down",
              "miqTarget": "ContainerNode",
              "severity": "HIGH",
              "url": "https://www.youtube.com/watch?v=dQw4w9WgXcQ"
            },
            "endsAt": "0001-01-01T00:00:00Z",
            "generatorURL": "http://prometheus-1018048397-9stw4:9090/graph?g0.expr=up%7Bjob%3D%22kubernetes-nodes%22%7D+%3D%3D+1&g0.tab=0",
            "labels": {
              "alertname": "Node Down",
              "beta_kubernetes_io_arch": "amd64",
              "beta_kubernetes_io_os": "linux",
              "instance": "ocp-compute01.10.35.48.142.nip.io",
              "job": "kubernetes-nodes",
              "kubernetes_io_hostname": "ocp-compute01.10.35.48.142.nip.io",
              "logging_infra_fluentd": "true",
              "region": "primary",
              "zone": "default"
            },
            "startsAt": "2017-08-02T10:16:30.457040707Z",
            "status": "firing"
          },
          {
            "annotations": {
              "description": "ocp-compute02.10.35.48.143.nip.io is down_d",
              "message": "ocp-compute02.10.35.48.143.nip.io is down",
              "miqTarget": "ContainerNode",
              "severity": "HIGH",
              "url": "https://www.youtube.com/watch?v=dQw4w9WgXcQ"
            },
            "endsAt": "0001-01-01T00:00:00Z",
            "generatorURL": "http://prometheus-1018048397-9stw4:9090/graph?g0.expr=up%7Bjob%3D%22kubernetes-nodes%22%7D+%3D%3D+1&g0.tab=0",
            "labels": {
              "alertname": "Node Down",
              "beta_kubernetes_io_arch": "amd64",
              "beta_kubernetes_io_os": "linux",
              "instance": "ocp-compute02.10.35.48.143.nip.io",
              "job": "kubernetes-nodes",
              "kubernetes_io_hostname": "ocp-compute02.10.35.48.143.nip.io",
              "logging_infra_fluentd": "true",
              "region": "primary",
              "zone": "default"
            },
            "startsAt": "2017-08-02T10:16:30.457040707Z",
            "status": "firing"
          },
        ]
      }
    }
}

moolitayer · 2017-08-03T13:24:13Z

@miq-bot add_label events, enhancement

moolitayer · 2017-08-03T18:01:19Z

@Fryguy @jrafanie does this make sense?

moolitayer · 2017-08-08T12:51:07Z

@Fryguy @jrafanie Please take a look. Context explained in first comment

jrafanie · 2017-08-10T17:46:24Z

This makes sense @moolitayer. Can you add a test that recreates the issue and demonstrates how your change fixes it?

Fryguy · 2017-09-06T18:39:04Z

Seems good to me. Agree on adding a spec.

moolitayer · 2017-09-15T15:32:38Z

Thanks, done, PTAL @jrafanie @Fryguy

moolitayer · 2017-09-15T15:35:08Z

spec/models/ems_event_spec.rb

@@ -196,6 +197,27 @@
          expect(new_event.availability_zone_id).to eq @availability_zone.id
        end
      end
+


".add" context is nested under "with availability zones"
Not sure if it's less confusing to add the new context here or create another top level ".add" context

moolitayer · 2017-09-28T20:22:40Z

@jrafanie @Fryguy I think this is ready. PTAL
(rubocop issue ins't from this change)

jrafanie · 2017-09-28T21:44:05Z

spec/models/ems_event_spec.rb

@@ -196,6 +198,24 @@
          expect(new_event.availability_zone_id).to eq @availability_zone.id
        end
      end
+
+      context "when two events have the same timestamp" do
+        context "when events belong to the same object" do


Were you going to add another test here? If not, we can probably collapse these two contexts into one.

jrafanie · 2017-09-28T21:45:38Z

spec/models/ems_event_spec.rb

+          it "should add only one of them" do
+            EmsEvent.add(@ems.id, @event_hash)
+            second = EmsEvent.add(@ems.id, @event_hash)
+            expect(second).to be_nil


Should we check that there is only one EmsEvent row for the event hash details instead of checking the return value from add?

jrafanie · 2017-09-28T21:46:05Z

spec/models/ems_event_spec.rb

+          it "should add them both" do
+            EmsEvent.add(@ems.id, @event_hash)
+            second = EmsEvent.add(@ems.id, @event_hash.update(:target_id => 1))
+            expect(second).not_to be_nil


Same as above.

jrafanie · 2017-09-28T21:46:44Z

sorry @moolitayer, I didn't see this update. I had a few minor comments on the tests. Please keep mentioning me until I see it and respond 😉

miq-bot · 2017-09-28T23:12:57Z

Checked commit moolitayer@e368740 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 1 offense detected

spec/models/ems_event_spec.rb

❗ - Line 176, Col 32 - Rails/TimeZone - Do not use Time.now without zone. Use one of Time.zone.now, Time.current, Time.now.in_time_zone, Time.now.utc, Time.now.getlocal, Time.now.iso8601, Time.now.jisx0301, Time.now.rfc3339, Time.now.to_i, Time.now.to_f instead.

jrafanie

Looks good 👍

moolitayer force-pushed the event_uniqueness branch from 11fa648 to f9b3c9b Compare August 3, 2017 12:18

moolitayer mentioned this pull request Aug 3, 2017

Prometheus alerts ManageIQ/manageiq-providers-kubernetes#40

Merged

miq-bot added enhancement core/events labels Aug 3, 2017

chessbyte assigned Fryguy Aug 3, 2017

moolitayer force-pushed the event_uniqueness branch from f9b3c9b to af43ef6 Compare September 15, 2017 15:32

moolitayer commented Sep 15, 2017

View reviewed changes

moolitayer force-pushed the event_uniqueness branch 3 times, most recently from 2adfcc9 to 450e8c0 Compare September 28, 2017 20:10

jrafanie reviewed Sep 28, 2017

View reviewed changes

moolitayer force-pushed the event_uniqueness branch 3 times, most recently from aa29dd7 to 1d4ba5a Compare September 28, 2017 22:50

Add target to event existence check

e368740

moolitayer force-pushed the event_uniqueness branch from 1d4ba5a to e368740 Compare September 28, 2017 23:08

jrafanie approved these changes Sep 29, 2017

View reviewed changes

jrafanie merged commit 39d657f into ManageIQ:master Sep 29, 2017

jrafanie added this to the Sprint 70 Ending Oct 2, 2017 milestone Sep 29, 2017

jrafanie assigned jrafanie and unassigned Fryguy Sep 29, 2017

moolitayer mentioned this pull request Oct 29, 2017

Use event ems_ref #16339

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add target to event existence check #15719

Add target to event existence check #15719

moolitayer commented Aug 3, 2017 •

edited

Loading

moolitayer commented Aug 3, 2017

moolitayer commented Aug 3, 2017

moolitayer commented Aug 8, 2017

jrafanie commented Aug 10, 2017

Fryguy commented Sep 6, 2017

moolitayer commented Sep 15, 2017

moolitayer Sep 15, 2017

moolitayer commented Sep 28, 2017

jrafanie Sep 28, 2017

jrafanie Sep 28, 2017

jrafanie Sep 28, 2017

jrafanie commented Sep 28, 2017 •

edited

Loading

miq-bot commented Sep 28, 2017

jrafanie left a comment

Add target to event existence check #15719

Add target to event existence check #15719

Conversation

moolitayer commented Aug 3, 2017 • edited Loading

moolitayer commented Aug 3, 2017

moolitayer commented Aug 3, 2017

moolitayer commented Aug 8, 2017

jrafanie commented Aug 10, 2017

Fryguy commented Sep 6, 2017

moolitayer commented Sep 15, 2017

moolitayer Sep 15, 2017

Choose a reason for hiding this comment

moolitayer commented Sep 28, 2017

jrafanie Sep 28, 2017

Choose a reason for hiding this comment

jrafanie Sep 28, 2017

Choose a reason for hiding this comment

jrafanie Sep 28, 2017

Choose a reason for hiding this comment

jrafanie commented Sep 28, 2017 • edited Loading

miq-bot commented Sep 28, 2017

jrafanie left a comment

Choose a reason for hiding this comment

moolitayer commented Aug 3, 2017 •

edited

Loading

jrafanie commented Sep 28, 2017 •

edited

Loading