Repository: zulip/zulip
Pull request: integrations: Add support for "Test plugin" in Sentry integration.
If a user tries to create a webhook using the Webhooks plugin in Sentry and uses the "Test plugin" to test the webhook, the server would send a 500 error, even though the integration worked perfectly. This led users to believe that the integration was not working. 

This PR fixes that behaviour and makes sure the sample event is handled properly.

Fixes: #26173 

<details>
<summary>Self-review checklist</summary>

<!-- Prior to submitting a PR, follow our step-by-step guide to review your own code:
https://zulip.readthedocs.io/en/latest/contributing/code-reviewing.html#how-to-review-code -->

<!-- Once you create the PR, check off all the steps below that you have completed.
If any of these steps are not relevant or you have not completed, leave them unchecked.-->

- [x] [Self-reviewed](https://zulip.readthedocs.io/en/latest/contributing/code-reviewing.html#how-to-review-code) 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](https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html)).

- [x] Each commit is a coherent idea.
- [x] 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.
</details>


Problem statement:
Sentry integration "Test Plugin" failure
Hello, I'm setting up Sentry.io integration and got this error when I tried "Test Plugin" (meaning "Test Outgoing Webhook to Zulip"); I'm assuming it's the response payload that came back from Zulip Cloud:

>"There was an internal error with the Plugin, {\"result\":\"error\",\"msg\":\"The 'Raven SDK' event isn't currently supported by the Sentry webhook\",\"webhook_name\":\"Sentry\",\"event_type\":\"Raven SDK\",\"code\":\"UNSUPPORTED_WEBHOOK_EVENT_TYPE\"}\n"

I'm not sure if there are any events that do work because I'm new to Sentry and not sure how to trigger test events other than the Test Plugin event.

**Zulip Server and web app version:**

- [x] Zulip Cloud (`*.zulipchat.com`)
- [ ] Zulip Server 7.0+
- [ ] Zulip Server 6.0+
- [ ] Zulip Server 5.0 or older
- [ ] Other or not sure

Hints from issue discussion:
@zulipbot add "area: integrations"
Hello @zulip/server-integrations members, this issue was labeled with the "area: integrations" label, so you may want to check it out!

<!-- areaLabelAddition -->

@aryairani Thanks for reporting this issue. 

This issue was recently fixed in 16563a321. I tried to reproduce this by pressing the **Send Test Notification** button while creating an alert rule and a notification was received as expected. I also inspected the received webhook and all seems to be alright.

Can you confirm if you are creating an "Alert Rule" as mentioned in the [documentation](https://zulip.com/integrations/doc/sentry)? 
Hi @sbansal1999,

This is under 
`https://<myorg>.sentry.io/settings/projects/<myproj>/plugins/webhooks/`
Pressing [Test Plugin], with Callback Url 
`https://<myzulip>.zulipchat.com/api/v1/external/sentry?api_key=<mykey>&stream=<mystream>`
Is this expected to work?

I'm working on getting permission to "Create New Integration"; fwiw the don't currently have permissions to create a custom integration for the whole org, I'm "only" admin for the project.

The webhook also appears under` https://<myorg>.sentry.io/settings/projects/<myproj>/alerts/`. Is this also not right?

Ok thanks!
Alright, I guess I got it sorted out, thanks for the hint.
I wonder if it might be worthwhile to add to a note that the Sentry Webhook integration is not the right way to configure Sentry alerts via Webhook to Zulip?
@aryairani  Our [Sentry documentation](https://zulip.com/integrations/doc/sentry) says:

>NOTE: Zulip also supports configuring this as a webhook in Sentry — which, while easier to configure (Navigate to Settings > Integrations > WebHooks) may not include the full breadth of event types.  

I tried creating a webhook using the `Integrations` list -- as you did. I was able to trigger issues on my sample project and then the events resulted in correct messages being sent by the bot. But as you pointed out when testing it by using the "Test Plugin" button it returns an error. 

So practically speaking, the webhook will still be able to receive events related to issues but won't respond to the test 

The only problematic thing here is that Sentry doesn't explicitly mention its condition for the sample event; hence making it difficult to identify them. So, I don't think there is a need to delete this from the documentation.
I see, sorry I missed that. Fwiw I find the integration docs to have a bit of a "wall of text" vibe, which makes it hard to follow (not just the Sentry integration docs); but I'm not a designer myself so I don't have any concrete suggestions on that front. 

Thanks for your help! Should I close the ticket?
#25976 is actively being worked on, which should help address the big block of text in various integrations documentation pages for generating the URL. But until that's done, I don't think there's much to be done about that block of text in step 2.

I did a little revision pass of the Sentry specific instructions (steps 3 and 4), and have #26224 up with an updated version of the current documentation that breaks up that text a bit and emphasized that warning/note about the alternative implementation via Sentry webhook.
@laurynmm Thanks for making some needed changes to the documentation. 

I also went ahead and opened #26231 to fix the behavior that was reported in the issue. After this is merged whenever the user will test the webhook using the "Test plugin" button (which the author also did and is also a documented way), the integration will not return a 500 and will lead to a meaningful notification being sent.

Commit message: integrations: Add Raven SDK test to Sentry Integration.

Patch to review:
diff --git a/zerver/webhooks/sentry/fixtures/raven_sdk_python_event.json b/zerver/webhooks/sentry/fixtures/raven_sdk_python_event.json
new file mode 100644
index 0000000000000..d267f1561569f
--- /dev/null
+++ b/zerver/webhooks/sentry/fixtures/raven_sdk_python_event.json
@@ -0,0 +1,134 @@
+{
+    "id": "4311163364",
+    "project": "python-raven",
+    "project_name": "python-raven",
+    "project_slug": "python-raven",
+    "logger": null,
+    "level": "error",
+    "culprit": "__main__ in <module>",
+    "message": "ZeroDivisionError: division by zero",
+    "url": "https://nitk-46.sentry.io/issues/4311163364/?referrer=webhooks_plugin",
+    "triggering_rules": [
+        "a"
+    ],
+    "event": {
+        "event_id": "b465c7ca9581475093a6f5c3c6a523d2",
+        "level": "error",
+        "version": "6",
+        "type": "error",
+        "fingerprint": [
+            "{{ default }}"
+        ],
+        "culprit": "__main__ in <module>",
+        "logentry": {
+            "formatted": "ZeroDivisionError: division by zero"
+        },
+        "logger": "",
+        "modules": {
+            "python": "3.10.6"
+        },
+        "platform": "python",
+        "timestamp": 1689194871.0,
+        "received": 1689194872.041476,
+        "exception": {
+            "values": [
+                {
+                    "type": "ZeroDivisionError",
+                    "value": "division by zero",
+                    "module": "builtins",
+                    "stacktrace": {
+                        "frames": [
+                            {
+                                "function": "<module>",
+                                "module": "__main__",
+                                "filename": "sentry/temp.py",
+                                "abs_path": "/home/sbansal1999/p/sentry/temp.py",
+                                "lineno": 6,
+                                "pre_context": [
+                                    "from raven import Client",
+                                    "",
+                                    "client = Client('https://6130e69dc19e426e87deaa54744be1cd@o4505261800423424.ingest.sentry.io/4505518370586624')",
+                                    "",
+                                    "try:"
+                                ],
+                                "context_line": "    1 / 0",
+                                "post_context": [
+                                    "except ZeroDivisionError:",
+                                    "    client.captureException()"
+                                ],
+                                "in_app": false,
+                                "vars": {
+                                    "Client": "<class 'raven.base.Client'>",
+                                    "__annotations__": {},
+                                    "__builtins__": "<module 'builtins' (built-in)>",
+                                    "__cached__": null,
+                                    "__doc__": null,
+                                    "__file__": "'/home/sbansal1999/p/sentry/temp.py'",
+                                    "__loader__": "<_frozen_importlib_external.SourceFileLoader object at 0x7fd40cfdd930>",
+                                    "__name__": "'__main__'",
+                                    "__package__": null,
+                                    "__spec__": null,
+                                    "client": "<raven.base.Client object at 0x7fd40d107c10>"
+                                },
+                                "data": {
+                                    "orig_in_app": -1
+                                }
+                            }
+                        ]
+                    }
+                }
+            ]
+        },
+        "tags": [
+            [
+                "level",
+                "error"
+            ],
+            [
+                "server_name",
+                "sbansal1999-OMEN-Laptop-15-en0xxx"
+            ]
+        ],
+        "extra": {
+            "sys.argv": [
+                "'temp.py'"
+            ]
+        },
+        "sdk": {
+            "name": "raven-python",
+            "version": "6.10.0"
+        },
+        "ingest_path": [
+            {
+                "version": "23.6.2",
+                "public_key": "XE7QiyuNlja9PZ7I9qJlwQotzecWrUIN91BAO7Q5R38"
+            }
+        ],
+        "key_id": "3267509",
+        "project": 4505518370586624,
+        "grouping_config": {
+            "enhancements": "eJybzDRxc15qeXFJZU6qlZGBkbGugaGuoeEEAHJMCAM",
+            "id": "newstyle:2023-01-11"
+        },
+        "_metrics": {
+            "bytes.ingested.event": 1549,
+            "bytes.stored.event": 2356
+        },
+        "_ref": 4505518370586624,
+        "_ref_version": 2,
+        "hashes": [
+            "cac8c3af9240e70448922144a14f55b3"
+        ],
+        "location": "sentry/temp.py",
+        "metadata": {
+            "display_title_with_tree_label": false,
+            "filename": "sentry/temp.py",
+            "function": "<module>",
+            "type": "ZeroDivisionError",
+            "value": "division by zero"
+        },
+        "nodestore_insert": 1689194873.112506,
+        "title": "ZeroDivisionError: division by zero",
+        "id": "b465c7ca9581475093a6f5c3c6a523d2"
+    }
+}
diff --git a/zerver/webhooks/sentry/fixtures/sample_event.json b/zerver/webhooks/sentry/fixtures/sample_event_through_alert.json
similarity index 100%
rename from zerver/webhooks/sentry/fixtures/sample_event.json
rename to zerver/webhooks/sentry/fixtures/sample_event_through_alert.json
diff --git a/zerver/webhooks/sentry/tests.py b/zerver/webhooks/sentry/tests.py
index 22d886eeff721..b1f342ee18e3c 100644
--- a/zerver/webhooks/sentry/tests.py
+++ b/zerver/webhooks/sentry/tests.py
@@ -251,7 +251,7 @@ def test_deprecated_exception_message(self) -> None:
 ```"""
         self.check_webhook("deprecated_exception_message", expected_topic, expected_message)
 
-    def test_sample_event(self) -> None:
+    def test_sample_event_through_alert(self) -> None:
         expected_topic = "This is an example Python exception"
         expected_message = """\
 **New message event:** [This is an example Python exception](https://sentry.io/organizations/nitk-46/issues/4218258981/events/b6eff1a49b1f4132850b1238d968da70/)
@@ -259,4 +259,17 @@ def test_sample_event(self) -> None:
 **level:** error
 **timestamp:** 2023-05-31 11:06:16
 ```"""
-        self.check_webhook("sample_event", expected_topic, expected_message)
+        self.check_webhook("sample_event_through_alert", expected_topic, expected_message)
+
+    def test_raven_sdk_python_event(self) -> None:
+        payload = self.get_body("raven_sdk_python_event")
+        result = self.client_post(
+            self.url,
+            payload,
+            content_type="application/json",
+        )
+        self.assert_json_success(result)
+        self.assert_in_response(
+            "The 'Raven SDK' event isn't currently supported by the Sentry webhook; ignoring",
+            result,
+        )


Code review comments:
diff_hunk: @@ -81,6 +81,20 @@
 }
 
 
+def is_sample_event(event: Dict[str, Any]) -> bool:
+    # These are just some workarounds; there is no documented way to check
+    # if an event is a sample event or not.
+    tags = event.get("tags", [])
+    if ["sample_event", "yes"] in tags:
+        return True
+    title = event.get("title", "")
+    if title == "This is an example Python exception":
+        return True
path: zerver/webhooks/sentry/view.py
text: @user1:
I actually wasn't suggesting that you remove the other check -- I said they "seem potentially redundant unless one knows the history here."  I feel that, if we could only use it, the tags solution is better because it's less likely to have false-positives.  I can see users typing `raise("This is an example Python exception")` in their code when trying it out, and I'd rather that we not _think_ it's s sample-from-Sentry.  Right now, the only effect is that we'd accept a Raven SDK submission, but code does drift and if we, say, started doing other things based on `is_sample_event`, false-positives would get more dangerous.

Since we _have_ to use the title, I'm fine with dropping the tag support, but I'd prefer to adjust the "This is just a workaround" comment to make clearer that this is a _heuristic_ which may false-positive, so we should not rely on it for important behaviour decisions.

> When the integration is set through Alerts there is no response body shown to the user but when it is set through the Webhook plugin a response body is shown only when an error is returned. So I don't think we can hint the user as such.

Bummer.

> I think since it's clearly mentioned in the [documentation](https://zulip.com/integrations/doc/sentry) that setting the integration through the Webhook plugin is not the best idea hence, there is no need for this.

It's documented, but _people will not always read the documentation_ -- which is what made us need this PR in the first place. :)  So we should always think about if it's simple to have the user interface (here, through the Sentry UI) give good feedback.  So while there's no "need" for this, in that it's not a blocker, it certainly would have improved the product experience if we could.

> I think you meant to say via the Webhook plugin because if someone sets an integration through that that's when the integration doesn't provide all the events.

Ah, yeah, I said that backwards.  Sorry for any confusion!

@author:
> Since we have to use the title, I'm fine with dropping the tag support, but I'd prefer to adjust the "This is just a workaround" comment to make clearer that this is a heuristic which may false-positive, so we should not rely on it for important behaviour decisions.

Yes, I think that makes perfect sense.

>So while there's no "need" for this, in that it's not a blocker, it certainly would have improved the product experience if we could.

You are correct that would have certainly improved the overall experience. Since there is no way right now to do that, I feel we can leave this for now.