Prometheus alerts #40

moolitayer · 2017-06-14T15:10:15Z

Issue: ManageIQ/manageiq#14238
See issue for setup

miq-bot · 2017-07-24T08:59:05Z

This pull request is not mergeable. Please rebase and repush.

moolitayer · 2017-08-01T08:46:54Z

@cben @zgalor please review

moolitayer · 2017-08-01T09:14:02Z

@enoodle please review

zgalor

A few minor naming and privacy nits

zgalor · 2017-08-01T09:07:19Z

app/models/manageiq/providers/kubernetes/monitoring_manager/event_catcher/runner_mixin.rb

+    target = find_target(labels)
+    {
+      :ems_id              => @cfg[:ems_id],
+      :source              => 'DATAWAREHOUSE',


Should this be MONITORING ?

Unfortunately that would not work as DATAWAREHOUSE is our current hook into the event system. I have a PR renaming these two fields to MONITORING/monitoring_alert. I update here one that has been resolved

zgalor · 2017-08-01T09:09:29Z

app/models/manageiq/providers/kubernetes/monitoring_manager/event_catcher/runner_mixin.rb

+      :ems_id              => @cfg[:ems_id],
+      :source              => 'DATAWAREHOUSE',
+      :timestamp           => timestamp,
+      :event_type          => 'datawarehouse_alert',


see comment from 2 lines above

see previous response

zgalor · 2017-08-01T09:11:44Z

spec/models/manageiq/providers/kubernetes/monitoring_manager/event_catcher/runner_mixin_spec.rb

+    FactoryGirl.create(
+      :endpoint,
+      :role       => 'prometheus_alerts',
+      :hostname   => 'alerts-prometheus.10.35.48.141.nip.io',


maybe replace (here and in recordings) to fake hostname and token

zgalor · 2017-08-01T09:12:10Z

spec/models/manageiq/providers/kubernetes/monitoring_manager/event_catcher/runner_mixin_spec.rb

+    FactoryGirl.create(
+      :authentication,
+      :authtype => 'prometheus_alerts',
+      :auth_key => 'eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJrdWJlcm5ldGVzL3NlcnZpY2VhY2NvdW50Iiwia3ViZXJuZXRlcy5pby9zZXJ2aWNlYWNjb3VudC9uYW1lc3BhY2UiOiJtYW5hZ2VtZW50LWluZnJhIiwia3ViZXJuZXRlcy5pby9zZXJ2aWNlYWNjb3VudC9zZWNyZXQubmFtZSI6Im1hbmFnZW1lbnQtYWRtaW4tdG9rZW4tZzRscDciLCJrdWJlcm5ldGVzLmlvL3NlcnZpY2VhY2NvdW50L3NlcnZpY2UtYWNjb3VudC5uYW1lIjoibWFuYWdlbWVudC1hZG1pbiIsImt1YmVybmV0ZXMuaW8vc2VydmljZWFjY291bnQvc2VydmljZS1hY2NvdW50LnVpZCI6ImQxOWUzMDk4LTZmOGUtMTFlNy04YTQ1LTAwMWE0YTE2MjY4YyIsInN1YiI6InN5c3RlbTpzZXJ2aWNlYWNjb3VudDptYW5hZ2VtZW50LWluZnJhOm1hbmFnZW1lbnQtYWRtaW4ifQ.m4ckxIoTj__rCFA25V8lurlapNgL6doCpP51Z0JA5VgiU9mRxdk8KGyCe6w_mhrh5xPO9UuiK_pYIIMPQ-lAijRaQrKITZ-smRMEcsqKU0uP10UliJIj7ICGxn7c2zDYQuxMOQwI4ELWPv4xBHVXkF0hBKhVCPWRmdnIwADIfQi4RqoN_1gQf8S2O8C_eDS-A1f5SUf1Bvhexs997NWFBVyKEUQKxdqAlHf59V3Zt2VaE2HUme_ppYvp5L9QSa-CEZU_XxmUwNeCzcgbzts8OMujuHB_8lwKcLy9H0k3_oh-EcOn4CvzVz6pTfh8HPDoFhqpnBHfLB4pnsRkoHbDrA',


see previous comment

zgalor · 2017-08-01T09:13:47Z

spec/models/manageiq/providers/kubernetes/monitoring_manager/event_catcher/stream_spec.rb

+    FactoryGirl.create(
+      :endpoint,
+      :role       => 'prometheus_alerts',
+      :hostname   => 'alerts-prometheus.10.35.48.141.nip.io',


maybe replace (here and in recordings) to fake hostname and token

zgalor · 2017-08-01T09:14:34Z

spec/models/manageiq/providers/kubernetes/monitoring_manager_spec.rb

@@ -14,7 +14,7 @@
  let(:prometheus_authentication) do
    FactoryGirl.create(
      :authentication,
-      :authtype => :prometheus_alerts,
+      :authtype => 'prometheus_alerts',
      :auth_key => 'eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJrdWJlcm5ldGVzL3NlcnZpY2VhY2NvdW50Iiwia3ViZXJuZXRlcy5pby9zZXJ2aWNlYWNjb3VudC9uYW1lc3BhY2UiOiJtYW5hZ2VtZW50LWluZnJhIiwia3ViZXJuZXRlcy5pby9zZXJ2aWNlYWNjb3VudC9zZWNyZXQubmFtZSI6Im1hbmFnZW1lbnQtYWRtaW4tdG9rZW4tMnBzZjMiLCJrdWJlcm5ldGVzLmlvL3NlcnZpY2VhY2NvdW50L3NlcnZpY2UtYWNjb3VudC5uYW1lIjoibWFuYWdlbWVudC1hZG1pbiIsImt1YmVybmV0ZXMuaW8vc2VydmljZWFjY291bnQvc2VydmljZS1hY2NvdW50LnVpZCI6IjJiMjg1MmIyLTZjNzAtMTFlNy1hYTlmLTAwMWE0YTE2MjYxOSIsInN1YiI6InN5c3RlbTpzZXJ2aWNlYWNjb3VudDptYW5hZ2VtZW50LWluZnJhOm1hbmFnZW1lbnQtYWRtaW4ifQ.OSh7pgdRXAIUh8hPipfj_me-T3cwI_DsqQUSV1yYo1qvGEd1Aa-oVaBAeKsbwjCDhcNw2nNTWkdeYoy4-MyoTjleZbvdsbtSgs84LCPdLfaVd7NXBiXtNj6o4a2tbcE_GUTBiOEyZfzFh5pwM1n9BbU6qwAvf5B-uiAuGI1VZXyiUWtlspNAEPOy3awFNt9vausNahfMRox9MLB62BYzj2NV36inpNY_UWMNV0X0Q1VnZWO6-v28JAkWhuqaRHgSOUKV1FKJDY6R4rCGxt5BnVS6_81au80vouZmv0oR6kvDPWZo6IVPG8JCIpxK0liJW65pxKIBf7oOkEzhi3wy9w',


maybe replace (here and in recordings) to fake token

moolitayer · 2017-08-01T13:20:40Z

@ilackarms please review
(BTW since we are blocked on watch this is still using pulling)

moolitayer · 2017-08-02T11:43:01Z

code climate asks to use safe navigation operator (&.) available since ruby 2.3.
According to travis.yml here and in the main repo, we do not support 2.2 (which is in security maintenance phase) so that should be fine. I do not see any current usages of safe navigation although I would like to start using it.
cc @jrafanie @cben

moolitayer · 2017-08-02T11:46:14Z

@cben @zgalor @enoodle @simon3z needs your review please

cben · 2017-08-02T14:16:24Z

app/models/manageiq/providers/kubernetes/monitoring_manager/event_catcher/runner_mixin.rb

+    event = event.dup
+
+    annotations, labels = event["annotations"], event["labels"]
+    severity, event[:url] = annotations["severity"], annotations["url"]


IMHO these are paired randomly and could read better as separate assignments.

event[:url] write is hiding here in middle of line.

labels and its uses far below could be closer together.

cben · 2017-08-02T14:17:32Z

app/models/manageiq/providers/kubernetes/monitoring_manager/event_catcher/runner_mixin.rb

+
+    event[:severity] = parse_severity(severity)
+    event[:resolved] = event["status"] == 'resolved'
+    event[:ems_ref] = incident_identifier(event["startsAt"], labels)


event will contain a mixture of string and symbol keys. intentional?
all the symbol keys are added only so that they appear in full_data, right?

all the symbol keys are added only so that they appear in full_data, right?

Right

event will contain a mixture of string and symbol keys. intentional?

to be taken into account these should be symbols[1], if we care about all the keys having the same type we can call deep_symbolize_keys (or have the client return symbol keyed objects). WDYT @cben?

[1] https://github.com/moolitayer/manageiq/blob/408cc00831ae9fc669a79f6be002f8e6effc264f/app/models/ems_event.rb#L177-L183

cben · 2017-08-02T14:36:07Z

app/models/manageiq/providers/kubernetes/monitoring_manager/event_catcher/runner_mixin.rb

+  def incident_identifier(incident_start, labels)
+    # When event b resolves event a, they both have the same startAt.
+    # Labels are added to avoid having two incidents starting at the same time.
+    "#{incident_start}_#{labels.hash}"


.hash is the ruby method returning an integer, right?
It's not stable between ruby runs (randomized seed against DOS)! And obviously not between ruby versions.
You're persisting it, which sounds bad idea.

Can you use some canonic json / yaml serialization (or its SHA if length is a concern)?

jrafanie · 2017-08-02T16:14:01Z

code climate asks to use safe navigation operator (&.) available since ruby 2.3.
According to travis.yml here and in the main repo, we do not support 2.2 (which is in security maintenance phase) so that should be fine. I do not see any current usages of safe navigation although I would like to start using it.
cc @jrafanie @cben

@moolitayer we can't yet because we haven't officially dropped 2.2.

I haven't gotten getting this ruby 2.4 issue done to completion. Once that is complete, we'd drop support for 2.2 and only support 2.3 and 2.4. If you have feedback on which gems are complete from that list, or ones that are missing, I could use help making sure our gem ecosystem works on 2.4.

ilackarms · 2017-08-29T08:41:19Z

tested working based on 4.6 deployment efforts

miq-bot · 2017-08-30T15:59:27Z

This pull request is not mergeable. Please rebase and repush.

simon3z · 2017-08-31T10:34:53Z

tested working based on 4.6 deployment efforts

@ilackarms IIUC this still requires some manual activation?

ilackarms · 2017-09-03T09:15:24Z

@simon3z yes, need to manually create alert policy in Control dashboard

miq-bot · 2017-09-05T14:14:40Z

This pull request is not mergeable. Please rebase and repush.

miq-bot · 2017-09-27T09:14:07Z

Checked commit moolitayer@4cd2b3e with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
11 files checked, 0 offenses detected
Everything looks fine. 🏆

simon3z · 2017-10-05T15:23:35Z

app/models/manageiq/providers/kubernetes/monitoring_manager/event_catcher/runner_mixin.rb

+    event[:resolved] = event["status"] == "resolved"
+    timestamp = event["timestamp"]
+
+    target = find_target(labels)


@moolitayer all the target references below (e.g. target.id, etc.) are crashing when target is nil.
cc @ilackarms @joelddiaz

simon3z · 2017-10-16T14:41:52Z

app/models/manageiq/providers/kubernetes/monitoring_manager/event_catcher/runner_mixin.rb

+  end
+
+  def stop_event_monitor
+    @event_monitor_handle.stop unless @event_monitor_handle.nil?


@moolitayer there's a codeclimate comment to remember here for future fixes/refactorings:

Use safe navigation (&.) instead of checking if an object exists before calling the method.

See this comment: #40 (comment)

@moolitayer why can't you use try meanwhile?

moolitayer changed the title ~~Prometheus alerts~~ [WIP] Prometheus alerts Jun 14, 2017

moolitayer mentioned this pull request Jun 14, 2017

Multiple managers for Kubernetes #39

Closed

9 tasks

miq-bot added the wip label Jun 14, 2017

moolitayer force-pushed the prometheus_alerts branch 2 times, most recently from e12b8c4 to 497cedf Compare June 15, 2017 14:37

moolitayer force-pushed the prometheus_alerts branch from 497cedf to c00f211 Compare July 2, 2017 15:50

moolitayer changed the title ~~[WIP] Prometheus alerts~~ Prometheus alerts Jul 3, 2017

miq-bot removed the wip label Jul 3, 2017

moolitayer mentioned this pull request Jul 4, 2017

Adapt manageiq to new managers ManageIQ/manageiq#15506

Merged

moolitayer force-pushed the prometheus_alerts branch 5 times, most recently from c12f52f to 566740d Compare July 6, 2017 10:40

miq-bot added the unmergeable label Jul 24, 2017

moolitayer added enhancement events labels Aug 1, 2017

moolitayer force-pushed the prometheus_alerts branch from 566740d to a0d2b11 Compare August 1, 2017 08:32

miq-bot removed the unmergeable label Aug 1, 2017

moolitayer force-pushed the prometheus_alerts branch from a0d2b11 to c48c3bb Compare August 1, 2017 08:41

zgalor suggested changes Aug 1, 2017

View reviewed changes

moolitayer force-pushed the prometheus_alerts branch from c48c3bb to ad1b9c7 Compare August 2, 2017 11:28

cben reviewed Aug 2, 2017

View reviewed changes

moolitayer force-pushed the prometheus_alerts branch from 6f12185 to 3ca4123 Compare August 28, 2017 11:57

miq-bot added the unmergeable label Aug 30, 2017

moolitayer force-pushed the prometheus_alerts branch from 3ca4123 to 888c40b Compare August 31, 2017 11:59

miq-bot removed the unmergeable label Aug 31, 2017

moolitayer force-pushed the prometheus_alerts branch from 888c40b to 859efc7 Compare August 31, 2017 12:09

moolitayer force-pushed the prometheus_alerts branch 3 times, most recently from 8abe758 to 5b90a58 Compare September 4, 2017 10:43

miq-bot added the unmergeable label Sep 5, 2017

moolitayer force-pushed the prometheus_alerts branch from 5b90a58 to e292629 Compare September 6, 2017 10:50

miq-bot removed the unmergeable label Sep 6, 2017

moolitayer force-pushed the prometheus_alerts branch 3 times, most recently from 7d18a37 to e79c74f Compare September 9, 2017 22:38

moolitayer force-pushed the prometheus_alerts branch from e79c74f to 01d1a18 Compare September 27, 2017 08:57

Add prometheus event collection

4cd2b3e

moolitayer force-pushed the prometheus_alerts branch from 01d1a18 to 4cd2b3e Compare September 27, 2017 09:07

simon3z reviewed Oct 5, 2017

View reviewed changes

simon3z reviewed Oct 16, 2017

View reviewed changes

simon3z merged commit 446495e into ManageIQ:master Oct 16, 2017

This was referenced Oct 16, 2017

Require 'faraday_middleware' in monitoring_manager_mixin.rb #125

Closed

Failing test without 'application_helper' #131

Closed

agrare added this to the Sprint 71 Ending Oct 16, 2017 milestone Oct 16, 2017

moolitayer mentioned this pull request Oct 29, 2017

Collect ems targeted Prometheus events #140

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prometheus alerts #40

Prometheus alerts #40

moolitayer commented Jun 14, 2017 •

edited

Loading

miq-bot commented Jul 24, 2017

moolitayer commented Aug 1, 2017

moolitayer commented Aug 1, 2017

zgalor left a comment

zgalor Aug 1, 2017

moolitayer Aug 2, 2017

zgalor Aug 1, 2017

moolitayer Aug 2, 2017

zgalor Aug 1, 2017

zgalor Aug 1, 2017

zgalor Aug 1, 2017

zgalor Aug 1, 2017

moolitayer commented Aug 1, 2017

moolitayer commented Aug 2, 2017 •

edited

Loading

moolitayer commented Aug 2, 2017

cben Aug 2, 2017

cben Aug 2, 2017

moolitayer Aug 2, 2017

cben Aug 2, 2017

jrafanie commented Aug 2, 2017

ilackarms commented Aug 29, 2017

miq-bot commented Aug 30, 2017

simon3z commented Aug 31, 2017

ilackarms commented Sep 3, 2017

miq-bot commented Sep 5, 2017

miq-bot commented Sep 27, 2017

simon3z Oct 5, 2017

simon3z Oct 16, 2017

moolitayer Oct 16, 2017

simon3z Oct 16, 2017

Prometheus alerts #40

Prometheus alerts #40

Conversation

moolitayer commented Jun 14, 2017 • edited Loading

miq-bot commented Jul 24, 2017

moolitayer commented Aug 1, 2017

moolitayer commented Aug 1, 2017

zgalor 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

Choose a reason for hiding this comment

moolitayer commented Aug 1, 2017

moolitayer commented Aug 2, 2017 • edited Loading

moolitayer commented Aug 2, 2017

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

jrafanie commented Aug 2, 2017

ilackarms commented Aug 29, 2017

miq-bot commented Aug 30, 2017

simon3z commented Aug 31, 2017

ilackarms commented Sep 3, 2017

miq-bot commented Sep 5, 2017

miq-bot commented Sep 27, 2017

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

moolitayer commented Jun 14, 2017 •

edited

Loading

moolitayer commented Aug 2, 2017 •

edited

Loading