Update signal-cli-rest-api with more verbose error messages and report them #1696

mattwr18 · 2023-09-12T15:52:50Z

This changes how we report errors with Signal. Previously we just called response.value, which we know will raise an exception if the response is not successful. We then rescued the exception and provided as much context as we could.

If this is part of the error message, we want to parse out the challenge token and tag the error with this token so that we can set up a Sentry filter to send this error to a specific channel whose only responsibility will be to alert a dev they need to manually solve a captcha a run signal-cli submitRateLimitChallenge --challenge-token --captcha

docker-compose.yml

roschaefer · 2023-09-12T16:48:07Z

app/adapters/signal_adapter/outbound/text.rb

+                 else
+                   {}
+                 end
+          ErrorNotifier.report(exception, context: context, tags: tags)


I would include a default here for every other response code.

To be honest, as I wrote to you in Slack, I think this is overkill and makes it harder to maintain.
The error message now, since you added the stdout to the output can contain several different clues as to why the message failed to send and I don't like the idea of parsing the error messages for several different reasons and doing different things based on that information.

For example, we now have the full message that a number is not registered with Signal any longer if a contributor unregistered their number.

I think it is enough to send the entire message to sentry, as I did:

error_message = JSON.parse(response.body)['error'] exception = SignalAdapter::BadRequestError.new(error_code: response.code, message: error_message)

and continue sending the errors to the #monitoring channel. This channel is for actionable errors that a dev should look into/do something about.

We have another channel #support for non-actionable errors that might require some support from a non-dev.

roschaefer · 2023-09-12T16:51:41Z

app/adapters/signal_adapter/outbound/text.rb

@@ -19,14 +19,7 @@ def perform(recipient:, text:)
        response = Net::HTTP.start(url.host, url.port) do |http|
          http.request(request)
        end
-        response.value # may raise exception
-      rescue Net::HTTPClientException => e


If you remove the rescue then you will not even get to handle_response if an exception is raised.

Ok, I see from the comment now that response.value may raise the error and it was also removed, nevermind. But I find the behaviour surprising.

Yeah, I find the behavior as it exists surprising, to be honest. We call response.value because if it is an unsuccessful response calling .value on it raises an exception, we even explain that the only purpose for line 22 is that it may raise an exception.

I think a better way to handle it is to either check the class of the response, or check its code to determine whether the response was successful, or not.

roschaefer

The PR looks good! Only thing is I would expect a unit test here.

mattwr18 · 2023-09-13T17:30:45Z

The PR looks good! Only thing is I would expect a unit test here.

There were actually already unit tests for the sad path, which were failing after the refactor, I've updated them. See 9ad81b3

- If this is part of the error message, we want to parse out the challenge token and tag the error with this token so that we can set up a Sentry filter to send this error to a specific channel whose only responsibility will be to alert a dev they need to manually solve a captcha a run signal-cli submitRateLimitChallenge --challenge-token <TOKEN> --captcha <CAPTCHA>

Co-authored-by: Robert Schäfer <git@roschaefer.de>

- We original talked about tagging the exception so it could be sent to a dedicated channel specifically for these rate limit challenge requests, but as we report all errors, I wanted to keep them all going to the #monitoring channel. We can monitor that channel with the new error message and solve the captcha.

I use the same pattern from this PR here: #1686 Looks like it's always better to pass `URI` instead of `string`: https://ruby-doc.org/stdlib-2.4.1/libdoc/net/http/rdoc/Net/HTTPGenericRequest.html Because `@uri` and `@host` get initialized.

roschaefer · 2023-09-15T15:12:45Z

app/adapters/signal_adapter/outbound/file.rb

-        url = URI.parse("#{Setting.signal_cli_rest_api_endpoint}/v2/send")
-        request = Net::HTTP::Post.new(url.to_s, {
+        uri = URI.parse("#{Setting.signal_cli_rest_api_endpoint}/v2/send")
+        request = Net::HTTP::Post.new(uri, {


It matters that we pass URI not string.

Ok, this is just a copy/paste that survived from code you added d521f28#diff-2e66abeab283cf6d1ae8e78dbcd46410ae0b8eb01361bc44708e1426c6f89f28R31

I don't know why it matters. Maybe, you could further explain

roschaefer · 2023-09-15T15:19:06Z

app/adapters/signal_adapter/api.rb

+
+module SignalAdapter
+  class Api
+    class << self


I guess a class with a static method will do. ErrorNotifier is built the same way`.

Sure, don't know why I didn't think of that hehe

roschaefer

Code-wise, this can be merged. The only critical issue is which image we're going to deploy. Yesterday I was convinced by @mattwr18 to use our own image, because upstream has also unrelated changes, including newer version of signal-cli which causes issues when downgrading.

roschaefer · 2023-09-15T15:20:34Z

app/adapters/signal_adapter/bad_request_error.rb

+module SignalAdapter
+  class BadRequestError < StandardError
+    def initialize(error_code:, message:)
+      super("Message was not delivered with error code `#{error_code}` and message `#{message}`")


This might become a little unreadable on Sentry in case of multi-line messages, but let's see..

roschaefer · 2023-09-15T15:21:01Z

docker-compose.yml

@@ -26,7 +26,7 @@ services:
      - postgres_data:/var/lib/postgresql/data

  signal:
-    image: bbernhard/signal-cli-rest-api:0.67
+    image: bbernhard/signal-cli-rest-api:0.119-dev


@mattwr18 Are we going to roll our own image or not?

Don't think so. There is already a pre-release https://github.com/bbernhard/signal-cli-rest-api/releases/tag/0.68-pre

There, he advises to use this image for testing things out.

…xt_to_signal_errors" This reverts commit a5ccdb8, reversing changes made to c985028.

roschaefer reviewed Sep 12, 2023

View reviewed changes

docker-compose.yml Outdated Show resolved Hide resolved

roschaefer reviewed Sep 12, 2023

View reviewed changes

roschaefer requested changes Sep 12, 2023

View reviewed changes

mattwr18 changed the title ~~Inspect Signal error message for challenge token keyword~~ Update signal-cli-rest-api with more verbose error messages and report them Sep 13, 2023

mattwr18 and others added 4 commits September 15, 2023 16:09

Favor official docker image from bbernhard until official release

b0b5417

Co-authored-by: Robert Schäfer <git@roschaefer.de>

Update specs after refactor

60dc5fc

roschaefer force-pushed the 1386_add_more_context_to_signal_errors branch from 9ad81b3 to 43e2833 Compare September 15, 2023 14:34

refactor: DRY handle_response

7cea0c3

I use the same pattern from this PR here: #1686 Looks like it's always better to pass `URI` instead of `string`: https://ruby-doc.org/stdlib-2.4.1/libdoc/net/http/rdoc/Net/HTTPGenericRequest.html Because `@uri` and `@host` get initialized.

roschaefer force-pushed the 1386_add_more_context_to_signal_errors branch from 43e2833 to 7cea0c3 Compare September 15, 2023 15:12

roschaefer reviewed Sep 15, 2023

View reviewed changes

roschaefer approved these changes Sep 15, 2023

View reviewed changes

mattwr18 merged commit a5ccdb8 into main Sep 18, 2023
1 check passed

mattwr18 deleted the 1386_add_more_context_to_signal_errors branch September 18, 2023 18:51

mattwr18 added a commit that referenced this pull request Sep 18, 2023

Revert "Merge pull request #1696 from tactilenews/1386_add_more_conte…

e1a8a8e

…xt_to_signal_errors" This reverts commit a5ccdb8, reversing changes made to c985028.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update signal-cli-rest-api with more verbose error messages and report them #1696

Update signal-cli-rest-api with more verbose error messages and report them #1696

mattwr18 commented Sep 12, 2023

roschaefer Sep 12, 2023

mattwr18 Sep 12, 2023

roschaefer Sep 12, 2023

roschaefer Sep 12, 2023

mattwr18 Sep 12, 2023

roschaefer left a comment

mattwr18 commented Sep 13, 2023

roschaefer Sep 15, 2023

mattwr18 Sep 18, 2023

roschaefer Sep 15, 2023

mattwr18 Sep 18, 2023

roschaefer left a comment

roschaefer Sep 15, 2023

roschaefer Sep 15, 2023

mattwr18 Sep 18, 2023

Update signal-cli-rest-api with more verbose error messages and report them #1696

Update signal-cli-rest-api with more verbose error messages and report them #1696

Conversation

mattwr18 commented Sep 12, 2023

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

roschaefer left a comment

Choose a reason for hiding this comment

mattwr18 commented Sep 13, 2023

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

roschaefer 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