[SE-3136] Add client instance name to New Relic monitoring condition …

…name (#640)

* feat(monitoring): Add client instance name to New Relic monitoring condition name

To be able to find root cause of incidents faster and improve the investigation experience, adding the client's instance name to the monitoring condition's name. In case the name is too long, it will be truncated due to limitations.

* test(monitoring): Add extra test case to make sure that monitoring name is shortened if needed

* style: Reformat the new unit test to match the quality standards

instance/models/mixins/openedx_monitoring.py

@@ -22,9 +22,12 @@
 
 # Imports #####################################################################
 
+import textwrap
+
 from django.conf import settings
 from django.db import models
 from django.db.models import ProtectedError
+from django.utils.html import escape
 
 from instance import newrelic
 
@@ -65,6 +68,26 @@ def enable_monitoring(self):
         self._update_url_monitors(alert_policy)
         self._set_email_alerts(alert_policy)
 
+    @staticmethod
+    def _generate_monitor_condition_name(short_name: str, instance_name: str) -> str:
+        """
+        Generate monitor condition name for the given URL and short name of the instance.
+
+        Based on the documentation of New Relic, the monitor name should be less than
+        or equal to 64 chars. The shortening will not split a word into two, instead it
+        will skip the word which would be splitted.
+
+        Also, based on New Relic's documentation, the names cannot contain angle brackets
+        ("<" and ">"), so we need to escape them in case the instance names would contain any.
+        """
+
+        monitoring_name = "{short} of {instance}".format(
+            short=short_name,
+            instance=escape(instance_name)
+        )
+
+        return textwrap.shorten(monitoring_name, 64, placeholder="...")
+
     def _update_url_monitors(self, alert_policy):
         """
         Create a monitor for each URL to monitor, and delete the other monitors.
@@ -83,9 +106,8 @@ def _update_url_monitors(self, alert_policy):
                 # and create one if it doesn't. This helps when the alert condition
                 # wasn't created in a previous invocation due to some issue.
                 if not monitor.new_relic_alert_conditions.exists():
-                    alert_condition_id = newrelic.add_alert_nrql_condition(
-                        alert_policy.id, url, urls_to_monitor_dict[url]
-                    )
+                    monitor_name = self._generate_monitor_condition_name(urls_to_monitor_dict[url], self.name)
+                    alert_condition_id = newrelic.add_alert_nrql_condition(alert_policy.id, url, monitor_name)
                     monitor.new_relic_alert_conditions.create(id=alert_condition_id, alert_policy=alert_policy)
             else:
                 self.logger.info('Deleting New Relic Synthetics monitor for old public URL %s', url)
@@ -95,9 +117,8 @@ def _update_url_monitors(self, alert_policy):
             self.logger.info('Creating New Relic Synthetics monitor for new public URL %s', url)
             new_monitor_id = newrelic.create_synthetics_monitor(url)
             monitor = self.new_relic_availability_monitors.create(pk=new_monitor_id)
-            alert_condition_id = newrelic.add_alert_nrql_condition(
-                alert_policy.id, url, urls_to_monitor_dict[url]
-            )
+            monitor_name = self._generate_monitor_condition_name(urls_to_monitor_dict[url], self.name)
+            alert_condition_id = newrelic.add_alert_nrql_condition(alert_policy.id, url, monitor_name)
             monitor.new_relic_alert_conditions.create(id=alert_condition_id, alert_policy=alert_policy)
 
     def _set_email_alerts(self, alert_policy):

instance/tests/models/test_openedx_monitoring_mixins.py

@@ -106,24 +106,92 @@ def test_enable_monitoring(self, additional_monitoring_emails, expected_monitor_
         )
 
         # Check that alert emails have been set up
-        created_condition_urls = set()
+        created_conditions = set()
         list_of_emails_added = []
 
         for add_call in mock_newrelic.add_alert_nrql_condition.call_args_list:
-            created_condition_urls.add(add_call[0][1])
+            created_conditions.add((add_call[0][1], add_call[0][2]))
 
         for add_call in mock_newrelic.add_email_notification_channel.call_args_list:
             list_of_emails_added.append(add_call[0][0])
 
         self.assertEqual(
-            created_condition_urls,
-            set([instance.url, instance.studio_url, instance.lms_preview_url, instance.lms_extended_heartbeat_url])
+            created_conditions,
+            set([
+                (instance.url, 'LMS of {}'.format(instance.name)),
+                (instance.studio_url, 'Studio of {}'.format(instance.name)),
+                (instance.lms_preview_url, 'Preview of {}'.format(instance.name)),
+                (instance.lms_extended_heartbeat_url, 'Extended heartbeat of {}'.format(instance.name)),
+            ])
         )
         self.assertEqual(set(list_of_emails_added), set(expected_monitor_emails))
         mock_newrelic.add_notification_channels_to_policy.assert_called_with(
             1, list(range(len(expected_monitor_emails)))
         )
 
+    @ddt.data(
+        # [additional_monitoring_emails, expected final email monitor list]
+        [[], ['admin@opencraft.com']],
+        [['other@opencraft.com'], ['admin@opencraft.com', 'other@opencraft.com']],
+    )
+    @ddt.unpack
+    @patch('instance.models.mixins.openedx_monitoring.newrelic')
+    def test_long_monitoring_condition_names(self, additional_emails, expected_emails, mock_newrelic, mock_consul):
+        """
+        Check that the `enable_monitoring` method creates New Relic Synthetics
+        monitors for each of the instance's public urls, and enables email
+        alerts even for those instance names which has extremely long names.
+        """
+        monitor_ids = [str(uuid4()) for i in range(4)]
+        mock_newrelic.get_synthetics_monitor.return_value = []
+        mock_newrelic.get_synthetics_notification_emails.return_value = []
+        mock_newrelic.create_synthetics_monitor.side_effect = monitor_ids
+        mock_newrelic.add_alert_policy.return_value = 1
+        mock_newrelic.add_alert_nrql_condition.side_effect = list(range(4))
+        mock_newrelic.add_email_notification_channel.side_effect = list(range(len(expected_emails)))
+        instance = OpenEdXInstanceFactory()
+        instance.name += " long" * 15  # Generate a very long instance name
+        instance.additional_monitoring_emails = additional_emails
+        instance.enable_monitoring()
+
+        # Check that the monitors have been created
+        mock_newrelic.delete_synthetics_monitor.assert_not_called()
+        mock_newrelic.create_synthetics_monitor.assert_has_calls([
+            call(instance.url),
+            call(instance.studio_url),
+            call(instance.lms_preview_url),
+            call(instance.lms_extended_heartbeat_url),
+        ], any_order=True)
+        self.assertCountEqual(
+            instance.new_relic_availability_monitors.values_list('pk', flat=True),
+            monitor_ids
+        )
+
+        # Check that alert emails have been set up
+        created_condition_urls = set()
+        created_condition_names = set()
+        list_of_emails_added = []
+
+        for add_call in mock_newrelic.add_alert_nrql_condition.call_args_list:
+            created_condition_urls.add(add_call[0][1])
+            created_condition_names.add(add_call[0][2])
+
+        for add_call in mock_newrelic.add_email_notification_channel.call_args_list:
+            list_of_emails_added.append(add_call[0][0])
+
+        for condition_name in created_condition_names:
+            self.assertTrue(condition_name.endswith("..."))
+            self.assertLessEqual(len(condition_name), 64)
+
+        self.assertEqual(created_condition_urls, set([
+            instance.url,
+            instance.studio_url,
+            instance.lms_preview_url,
+            instance.lms_extended_heartbeat_url,
+        ]))
+        self.assertEqual(set(list_of_emails_added), set(expected_emails))
+        mock_newrelic.add_notification_channels_to_policy.assert_called_with(1, list(range(len(expected_emails))))
+
     @patch('instance.models.mixins.openedx_monitoring.newrelic')
     def test_enable_monitoring_for_pre_new_relic_alerts_instances(self, mock_newrelic, mock_consul):
         """