feat: combine contents from GlobalStore when reporting.

[shalvah]

Fixes #825 (comment)

When using AsyncStore, the store is always undefined if you aren't inside an async context. Normally, when we enter an async context (ie start handling a request with withRequest, which calls ALS' run()), if an error happens, we're still in that context when handling it, so we can access store.breadcrumbs and friends. But if something unexpected happens that takes us out of that context (for instance, an error during our error-handling), we can't access store anymore. This PR fixes that by falling back to the GlobalStore if store is undefined.

Also did a refactor, so GlobalStore is a global singleton, like AsyncStore.

[shalvah]

@subzero10 could you help merging and publishing a new release when this is ready? I don't have publish permissions, and I frankly don't trust myself to build packages for browser and server without messing something up. 😅

[subzero10]

I suspect you had to do this because now there is a singleton of the GlobalStore which means it might hold the state of other Honeybadger clients right?
Perhaps, it would be better to revert to the original usage of the GlobalStore to make sure we have separate context for each Honeybadger client. What do you think?
For ALS, we opted for the singleton, because we get a separate context every time we call run().

[shalvah]

True. But the inconsistency always was a bit jarring. However, a big reason why it's a singleton is because we need to be able to recover breadcrumbs from before we switched to async. ie this flow:

store is Global
do stuff that creates breadcrumbs
enter async context
switch store to Async
error happens in async context
report in async context (gather breadcrumbs and other event data) <-- a
something goes wrong while reporting
report new thing in global context (gather breadcrumbs and other event data) <--b

On line a: when gathering breadcrumbs to report, we also need breadcrumbs that happened before we switched to Async. That's what this line does. But now that I think of it, we don't really need a singleton for that. We can do it earlier, by initialising the ALS store with the global one:

AsyncStore.run({ ...GlobalStore.getStore() }, () => {
  // code
});

But the second point is stage b: when we exit the async context, we lose access to the async store, so the only way to recover any breadcrumbs that happened before entering the async context is by having the global store be a singleton.

I think the singleton approach is fine, since the Honeybadger client is also a global singleton. But now that you mention it, I see that a user could manually create another client with factory(). I'll update that method to reset the context for the created client.

[subzero10]

In addBreadcrumb, you set the store GlobalStore if it's undefined. Shouldn't you do the same here? And perhaps wrap into a private function to re-use (call this.getStore() instead of this._store.getStore())?
My concern with this approach is that we may accidentally switch to a GlobalStore when we originally wanted the AsyncStore. Then again, we always set the store to AsyncStore before we to use it.

[shalvah]

I did this at first, but it's slightly more complicated. I went with a getStore() method, but it caused trouble, as some places mutated the store, while others didn't. I couldn't simply return a copy, because some locations wanted to mutate the store. For example, addBreadcrumb() definitely wants to add to the store, hence the switch.

I admit that it's confusing, but I decided to go with this for now, but I'll continue looking for improvements. I'll take another look now, so you can go ahead and release the main fix in the other PR.

The place you linked where we always use the async store is in AWS Lambda handlers. In regular apps, we don't do that, because an app may or may not be a web app, and errors can happen before we start handling requests (eg in our setup code).

[subzero10]

Same comment as above.

[subzero10]

@subzero10 could you help merging and publishing a new release when this is ready? I don't have publish permissions, and I frankly don't trust myself to build packages for browser and server without messing something up. 😅

Sure. Let's get it ready and I can publish the release.

[subzero10]

@shalvah Can we also add a unit test that tests the scenario you described here?

[shalvah]

Sure, will add. Also: I'd like to rename store, AsyncStore and GlobalStore to -storage. It's always confusing to me when we call store.getStore().

[shalvah]

Good call on the test @subzero10 . I totally forgot about the whole mutability problem in JS, so even the places I thought were getting a copy weren't. 😅

I've updated it now. Added test + now the factory() method instantiates a fresh store too.

[subzero10]

Hey @shalvah, I've been thinking about this PR for the last couple of days :)
Here's what I came up with:

• My original intention was to create an interface of a store that would be decided at runtime. The consumer (Honeybadger Client class) of this store would be detached from the implementation details of such store (be it AsyncLocalStore OR an in-memory store). I did not intend for both stores to be used from the same consumer.
• With the recent reports/findings, it seems that we must always keep a reference to the in-memory store (which we know we can always access) and use ALS store whenever possible.
• Therefore, I'm thinking that we would have to redesign the honeybadger client <> store relation to better match the new use cases.

How about the following:

• For every honeybadger client, we create instances of stores that are supported by the environment (private stores: Store[])
• The stores have order of precedence, which is their position in the array
• For browsers and older versions of nodejs, it would be an array of a single store (the in-memory store)
• For nodejs, it would be an array of two stores [new AsyncLocalStorage(), new InMemoryStorage()] (AsyncLocalStorage could be a singleton because of it's API usage, but I am ignoring that for now)
• When adding to the store, we add to the first one available in the list (ALS should be first). This means that if data is added outside of ALS, it would be always available. This could be a good or bad, depending on the error being reported, but probably not a big deal.
• When reading from the store, we merge data from both stores, with the keys from ALS taking precedence (overriding values from the in-memory store). Or we could decide only to read from the first available store (?)
• When clearing, we clear all available stores

What do you think? cc @joshuap

[shalvah]

I did not intend for both stores to be used from the same consumer.

Hmm, I don't see how that can be a goal, since we explicitly start out with the GlobalStore before switching to the Async store for web requests (and that is what we should be doing, as errors can happen before we start handling requests).

When adding to the store, we add to the first one available in the list (ALS should be first).

The problem is that ALS is not always available. It may or may not be accessible, depending on if we're in an async context or not.

I'm not really a fan of the new proposal, because it sounds a bit more complex at face value. But it seems to have some real merits. I'll try it out and see what it looks like.

[subzero10]

I did not intend for both stores to be used from the same consumer.

Hmm, I don't see how that can be a goal, since we explicitly start out with the GlobalStore before switching to the Async store for web requests (and that is what we should be doing, as errors can happen before we start handling requests).

Exactly. That's something I missed when I originally came up with the solution.

When adding to the store, we add to the first one available in the list (ALS should be first).

The problem is that ALS is not always available. It may or may not be accessible, depending on if we're in an async context or not.

I agree. That's why I propose that we keep an array of the stores and add to the first available. If ALS is not available, we add to the next store.

I'm not really a fan of the new proposal, because it sounds a bit more complex at face value. But it seems to have some real merits. I'll try it out and see what it looks like.

What part do you not like? If you are not happy with it, then maybe we shouldn't go for it :)

P.S. In fact, I even have in my mind the evolution of this:

• If my proposal above works, we could come up with a 3rd store, which would essentially be the encapsulation of a hybrid implementation of the other 2 stores. The end result of this would be that the Honeybadger Client class would be completely abstracted from the implementation details of any store (no checks in the client class whether the store is available or merging data from both stores in the client class).

[shalvah]

Oh, no worries. I suspect it may just be my resistance to change. I'll wait for the monorepo to get merged in, and give this another go. I think the new structure might provide room for some clarity there.