ProseMirror / prosemirror

The ProseMirror WYSIWYM editor
http://prosemirror.net/
MIT License
7.59k stars 335 forks source link

RangeError messages are not stable, causes monitoring pain #1409

Closed bradleyayers closed 1 year ago

bradleyayers commented 1 year ago

To preface, we have a set of patches that I'd like to try and upstream.

We use Sentry to monitor unhandled errors, and we find that errors with dynamic messages can be problematic as they are seen as "different problems" rather than "different instances of the same problem". The way we've solved this is to make error messages stable, and put dynamic values as properties on the error, for example here's a patch to prosemirror-model:

-    if (!(pos >= 0 && pos <= doc.content.size)) throw new RangeError("Position " + pos + " out of range")
+    if (!(pos >= 0 && pos <= doc.content.size)) {
+      let e = new RangeError("Position out of range");
+      e["pos"] = pos;
+      throw e;
+    }

We have quite a number of these, is there appetite for having these merged?

marijnh commented 1 year ago

No, sorry, that seems more of a problem with your monitoring than with our errors. Changing them like this will make them harder to read (you won't see the position in your crash/stack trace).

I feel like it should be possible to fish out the error constructor call line from the stack trace and use that as a key.

bradleyayers commented 1 year ago

That makes sense, I'll investigate that. https://docs.sentry.io/product/data-management-settings/event-grouping/fingerprint-rules/#errorvalue looks viable