Rollup hash does not consider custom Exception data

[MattJeanes]

Not sure if this is intentional or not but currently the GetHash function only includes the Exception.ToString() and optionally MachineName but it does not consider custom Exception metadata - for example (from opserver):

Source for the GetHash function: https://github.com/NickCraver/StackExchange.Exceptional/blob/107b8641ac12bd41ae9c3f1378509892034111ff/src/StackExchange.Exceptional.Shared/Error.cs#L230-L244

We find this information helpful to recover from what happened here but what seems to happen is that multiple errors with different information get rolled up into one and only one of the exception's message remains and the others are lost

That's cool if this was intended but thought I'd check, may be worth at least having an option for it like the MachineName

Happy to do a PR to implement this if you agree that it always should / should have an option to 😄

[NickCraver]

Yep, this is intentional. When considering a practical "what would this do?" standpoint, it's almost always the same cause of an error with identical call stacks erroring in the same window. So it's about fixing the problem...and doesn't really handle cleaning up. Admittedly, this is based somewhat on Stack Overflow scale of throwing tens of millions of errors in a very short time frame, so I'm not going to pretend my view isn't biased here :)

I get the recovery case though, that makes sense if you're using the log as a ledger of sorts. My regret here is the API as it stands (after reading your issue :). If we wanted to roll-up via: X, it'd have been a lot better if the options object had a [Flags] enum we could trivially extend and add what to roll up by. In that respect, adding a lot of booleans isn't ideal and becomes a crazy mess fast (including for user intellisense). Unfortunately, this API permeates several levels, from .Log() down to .GetHash() and in-between with publicly exposed members we'd have to double up the whole way down (to not be a breaking change).

I think this is both nice to have, and very messy to implement (in a non-breaking way). Here's what I imagine an API looking like:

• Instead of bool rollupPerServer
• We'd have rollupBy: Rollup.ByServer
• Or in your case: rollupBy: Rollup.ByServer | Rollup.ByCustomData

...naming TBD, I don't love those, just trying to convey the idea.

What do you think about such an API? We could then allow anything there over time, e.g. Rollup.ByUrl, etc. ... without breaking the API, duplication, or needing a major release as a result of the former.

Thoughts?

[MattJeanes]

I do like the idea of flags to control the behaviour of roll up, basically exactly what you suggested sounds perfect here and could be done without breaking changes. If I get some time I'll try and throw up a PR and we'll iterate on that until we're happy.

[NickCraver]

Though we can do it without a breaking change, technically, we'd have to dupe up a lot of methods and it'd dupe the user's intellisense for logging as well. I'd much rather do this in a 3.x release properly...but if you want to try a PR with minimal breaking and duplication, I'm all for taking a shot. We need to weigh maintenance and user confusion for such a change, though - just being up front there!

[MattJeanes]

Absolutely agree. Will keep in mind if I take a jab at this