Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: combine contents from GlobalStore when reporting. #845

Closed
wants to merge 4 commits into from
Closed

feat: combine contents from GlobalStore when reporting. #845

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6a430b3
fix: don't crash if store is undefined
shalvah Jul 25, 2022
a11e420
fix: always clear store when creating new client
shalvah Jul 26, 2022
e1caec4
fix: correctly copy GlobalStore's contents
shalvah Jul 26, 2022
3256bd0
fix: correctly copy GlobalStore's contents
shalvah Jul 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 19 additions & 11 deletions src/core/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ export default abstract class Client {

// First, we go with the global (shared) store.
// Webserver middleware can then switch to the AsyncStore for async context tracking.
this.__store = new GlobalStore({ context: {}, breadcrumbs: [] })
this.__store = GlobalStore
this.__transport = transport
this.logger = logger(this)
}
Expand Down Expand Up @@ -125,7 +125,9 @@ export default abstract class Client {
setContext(context: Record<string, unknown>): Client {
if (typeof context === 'object') {
const store = this.__store.getStore()
store.context = merge(store.context, context)
if (store) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

store.context = merge(store.context, context)
}
}
return this
}
Expand All @@ -134,6 +136,8 @@ export default abstract class Client {
this.logger.warn('Deprecation warning: `Honeybadger.resetContext()` has been deprecated; please use `Honeybadger.clear()` instead.')
const store = this.__store.getStore()

if (store === undefined) return this
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above.


if (typeof context === 'object' && context !== null) {
store.context = context
}
Expand All @@ -146,8 +150,10 @@ export default abstract class Client {

clear(): Client {
const store = this.__store.getStore()
store.context = {}
store.breadcrumbs = []
if (store) {
store.context = {}
store.breadcrumbs = []
}

return this
}
Expand Down Expand Up @@ -347,13 +353,17 @@ export default abstract class Client {
const category = opts.category || 'custom'
const timestamp = new Date().toISOString()

const store = this.__store.getStore()
let store = this.__store.getStore()
if (!store) {
this.__setStore(GlobalStore)
store = this.__store.getStore()
}
let breadcrumbs = store.breadcrumbs
breadcrumbs.push({
category: category as string,
message: message,
metadata: metadata as Record<string, unknown>,
timestamp: timestamp
message,
timestamp,
})

const limit = this.config.maxBreadcrumbs
Expand Down Expand Up @@ -426,11 +436,9 @@ export default abstract class Client {
*/
protected __getStoreContentsOrDefault(): DefaultStoreContents {
const existingStoreContents = this.__store.getStore();
const storeContents = existingStoreContents || {};
return {
context: {},
breadcrumbs: [],
...storeContents
...GlobalStore.getStore(),
...existingStoreContents || {}
};
}
}
12 changes: 9 additions & 3 deletions src/core/store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { HoneybadgerStore } from './types';
import { HoneybadgerStore, DefaultStoreContents } from './types';

export class GlobalStore<T> implements HoneybadgerStore<T> {
class SyncStore<T> implements HoneybadgerStore<T> {
private store: T

constructor(store: T) {
Expand All @@ -15,4 +15,10 @@ export class GlobalStore<T> implements HoneybadgerStore<T> {
this.store = store;
return callback(...args);
}
}

static create(): SyncStore<DefaultStoreContents> {
return new SyncStore({ context: {}, breadcrumbs: [] })
}
}

export const GlobalStore = SyncStore.create()
2 changes: 1 addition & 1 deletion src/server/async_store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ try {
Store = new AsyncLocalStorage()
}
catch (e) {
Store = new GlobalStore({ context: {}, breadcrumbs: [] })
Store = GlobalStore
}

export const AsyncStore = Store
1 change: 1 addition & 0 deletions test/unit/browser.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ describe('browser client', function () {
client = Singleton.factory({
logger: nullLogger()
});
client.clear()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// @ts-ignore - no need to test this in here
client.__getSourceFileHandler = null
Expand Down
1 change: 1 addition & 0 deletions test/unit/core/client.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ describe('client', function () {
environment: null
}, new TestTransport())
client.configure()
client.clear()
})

describe('getVersion', function () {
Expand Down
1 change: 1 addition & 0 deletions test/unit/server.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ describe('server client', function () {
logger: nullLogger(),
environment: null
})
client.clear()
})

it('inherits from base client', function () {
Expand Down