integrations: Update Newrelic integrations. #29730

PieterCK · 2024-04-13T14:20:01Z

📄 PR Overview

Originally, this PR aims to resume the work done on #27779 by applying suggested changes by reviewer in the original PR and developing new tests. However, the scope of the PR has widened due to the New Relic changes reported in #29729.

This PR's scope is now to adjust the whole Zulip New Relic integration for the newest New Relic updates. It also includes new tests, new notifications as well as added new capabilities to the integration.

Fixes: #29729

🛠️ Updates & New Features Implemented

Updated New Relic Logo & Bot Avatar:

Updated New Relic Doc

Restructured to better follow changes in #29592
Adjusted the doc for the New Relic "workflow and destinations" update
Included documentation for new features and updated payload

BLOCKER : I encountered an error when trying to include an example of New Relic payload in a code block in the docs 'Recommended Base Payload Template' section, which could add improvements to the integration if used by the user. CZO detailing the error.

New Feature: Support for New Relic Custom Payload

User can now include New Relic webhook custom fields in their New Relic Zulip notification.

The details and guide on how to use this feature is already written in the new doc under "Displaying Custom Fields" section. For technical details, this feature is handled in generate_custom_field_message() .

notification with custom field "Your Custom Payload" :

relevant payload part for the notification above:

notification with a couple of incorrectly configured custom payload :

relevant payload part for the notification above:

Provided a "Recommended" base payload

The out of the box New Relic payloads is very minimal. To benefit from the newest features, a recommended base payload is provided in the integration doc.

default base payload:

{
	"id": {{ json issueId }},
	"issueUrl": {{ json issuePageUrl }},
	"title": {{ json annotations.title.[0] }},
	"priority": {{ json priority }},
	"impactedEntities": {{json entitiesData.names}},
	"totalIncidents": {{json totalIncidents}},
	"state": {{ json state }},
	"trigger": {{ json triggerEvent }},
	"isCorrelated": {{ json isCorrelated }},
	"createdAt": {{ createdAt }},
	"updatedAt": {{ updatedAt }},
	"sources": {{ json accumulations.source }},
	"alertPolicyNames": {{ json accumulations.policyName }},
	"alertConditionNames": {{ json accumulations.conditionName }},
	"workflowName": {{ json workflowName }}
}

provided base payload:

{
	"id": {{ json issueId }},
	"issueUrl": {{ json issuePageUrl }},
	"title": {{ json annotations.title.[0] }},
	"priority": {{ json priority }},
	"impactedEntities": {{json entitiesData.names}},
	"totalIncidents": {{json totalIncidents}},
	"state": {{ json state }},
	"trigger": {{ json triggerEvent }},
	"isCorrelated": {{ json isCorrelated }},
	"createdAt": {{ createdAt }},
        "closedAt": {{ json closedAt }},
	"updatedAt": {{ updatedAt }},
	"sources": {{ json accumulations.source }},
	"alertPolicyNames": {{ json accumulations.policyName }},
	"alertConditionNames": {{ json accumulations.conditionName }},
	"workflowName": {{ json workflowName }},
        "acknowledgedAt": {{ json acknowledgedAt }},
        "acknowedgedBy":{{ json acknowledgedBy }},
        "owner": {{ json owner }},
        "zulipCustomFields": {
            "yourCustomPayload": "somedata123"
         }
}

Note: added a couple of event time stamp fields, "owner" field and setup the "zulipCustomField" dict

Reworked Notification Topic Name Format

Changed notification topic name format to enable user to follow alerts based on priority and incident state

Old (current) topic name format:
{alert_policies} {issue_id}
e.g: "(Golden Signals, Computer Health)(123-039-932-0321-393)"
New topic name format:
{state} {priority} priority
e.g: "Activated Critical Priority"

The rationale is to enable the user to follow specific streams they want based on incident priority and state. The old format was also created when Alert Policy was a single value and not a list, which it is now.

New Feature: Added "Fallback" Notification

Added a very minimal notification format and a warning informing the integration might be outdated or the user has tampered the payload too much (omitting required field).

fallback notification:

payload for the notification above:

Redesigned Integration Notifications

The new notification is designed to better present the updated and new payloads from recent New Relic update. It is also being discussed with a couple of other members and user: CZO

notification with the default new relic payload:

Note: the "Updated" notifications are "Acknowledged" incidents, but the default payload doesn't provide enough data

notification with the payload we recommend and provided:

Note: the same "Updated" payload are now enriched with infos such as "Acknowledged By" field and the "ACKNOWLEDGED" action

variety of incident priority:

variety of incident priority (opened):

variety of incident states:

Note: Officially, there are 3 states: ACTIVATED, CLOSED, CREATED. But in practice, ACTIVATED and CREATED is used synonimously, so only ACTIVATED and CLOSED are displayed.

variety of incident actions beside Closed & Activated:

Note: Actions are infered by matching the provided timestamps (see determine_status()). All actions/ status: Activated, Closed, Updated and Acknowledged.
This is different from incident 'state' (previous point)

notification with some custom payload:

Note: to minimize redundancy, see the "New Feature: Support for New Relic Custom Payload" section above

fallback notification:

Note: to minimize redundancy, see the "New Feature: Added 'Fallback' Notification" section above

🧑‍🏭 Issues & Concerns Encountered

concern: ~~table border in dark mode is almost invisible since the color is still black. Details in~~: CZO (ADDRESSED HERE: PR dark_theme_css: Fix border color for tables in message/chat. #29859)
bug: ~~as mentioned earlier, attempting to render dictionary in a specific format inside a code block in doc .md will result in error. Details in~~: CZO (ADDRESSED HERE: Issue Fix error when rendering fenced code block with wierdly formated dictionary in doc.md file #29857 )
suggestion: I believe other integrations could also benefit from implementing a type of "Fallback" notification in case the integration becomes outdated. As a form of "error reporting" to the user in a "user friendly" way, this could improve UX (instead of user having to figure out why their notification one day won't show up) and better reaction time to breaking changes from third party updates.

📖 Helpful Resources

Self-review checklist

Self-reviewed the changes for clarity and maintainability
(variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

Explains differences from previous plans (e.g., issue description).
Highlights technical choices and bugs encountered.
Calls out remaining decisions and concerns.
Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

Each commit is a coherent idea.
Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

Visual appearance of the changes.
Responsiveness and internationalization.
Strings and tooltips.
End-to-end functionality of buttons, interactions and flows.
Corner cases, error conditions, and easily imagined bugs.

zulipbot · 2024-04-13T14:20:05Z

Hello @PieterCK, it seems like you have referenced #29729 in your pull request description, but you have not referenced them in your commit message description(s). Referencing an issue in a commit message automatically closes the corresponding issue when the commit is merged, which makes the issue tracker easier to manage.

Please run git commit --amend in your command line client to amend your commit message description with Fixes #29729..

An example of a correctly-formatted commit:

commit fabd5e450374c8dde65ec35f02140383940fe146
Author: zulipbot
Date:   Sat Mar 18 13:42:40 2017 -0700

    pull requests: Check PR commits reference when issue is referenced.

    Fixes #51.

To learn how to write a great commit message, please refer to our guide.

alya · 2024-04-23T19:56:16Z

Thank you for the detailed description and screenshots!

alya · 2024-04-23T19:57:00Z

@laurynmm Are you up for doing the first round of review here? I skimmed the overview, but have not thought carefully about the details.

PieterCK · 2024-05-07T14:06:23Z

Update: Resolved conflict

laurynmm

@PieterCK - I did an initial review for this integration update. I didn't get through all of the change, but I figured you had enough feedback so far that you could go through my comments that I've got so far so this work can move forward.

Two notes not in review comments:

The svg file seems big, especially when compared to the one it's replacing.
The commit is from the original author, so you'll want to mark youself as the co-author. Or you'll want to update that so you're the author and mark the original author as a co-author.

Let me know if you have any questions about the above or about any of my comments. Thanks for taking on these extensive updates to this New Relic integration!

laurynmm · 2024-05-15T17:02:11Z