FluidFramework
FluidFramework copied to clipboard
Add common message prefix to assert messages
Description
Inspired by AB#7786
With the common prefix "FF Assert:" regardless of whether the message is string or hex code, we can update our alerts and queries that monitor for asserts to look for this prefix rather than regex on the hex format.
Two benefits:
- ~We can detect asserts before they're tagged, which doesn't happen until the code is released. We monitor stress test logs for asserts, but those monitors are blind until the release, which is kind of late.~ [edit: we can already do this by looking at the callstack]
- If some other code logs the same format (unlikely), we'll be able to distinguish it at a glance. And general usability for partners looking at our logs.
Breaking Changes
This will break any telemetry query/monitor that keys of the entire message being exactly only a three-digit Hex value.
Part of the linked ADO work item is to update the alert though, so this is ok.
Reviewer Guidance
I had wanted to update assert to throw a LoggingError so we can also get features like errorInstanceId and the ability to add props (maybe we'd use props to identify asserts instead of message), but that is pretty disruptive - it requires moving several classes/interfaces to core-utils or other places. I may put a follow-up PR out to do that though.
Pros
- I like it better than having an alert that has to parse the call stack of errors to determine if an error is an assert.
Cons
- Negligible bump to bundle size.
- Not sure about "FF Assert", feels like it might not be immediately clear to people other than us (who know how/why we use assertions that throw this). One can argue maybe it shouldn't matter because this telemetry is not particularly intended for consumers, but I think it's still part of the visible output/behavior of FF so I'd try to lean towards making it clear. But don't have great ideas that I like much better. "AssertError" / "FFAssertError" feel redundant since the event category will already be error. "AssertCode"... seems ok but don't love it, and feels like it'd have a higher (even if still low) chance of collision with errors from somewhere else.
Just my 2 cents. Overall I'm in favor.
This PR has been automatically marked as stale because it has had no activity for 60 days. It will be closed if no further activity occurs within 8 days of this comment. Thank you for your contributions to Fluid Framework!