Fixed analytics #834
Fixed analytics #834
Conversation
| } | ||
|
|
||
| capture(message: { event: string; properties?: Record<string | number, any>; }): void { | ||
| return super.capture({ distinctId: this.getPersistedProperty('anonymous_id'), ...message }); |
There was a problem hiding this comment.
Setting the distinctId to the anon will create a new user each time and won't allow for aliasing multiple times
There was a problem hiding this comment.
I think we might just need to override this method and use anon instead of distinct when necessary:
https://github.com/PostHog/posthog-js-lite/blob/cb050098714676b00ddbdae3a5a4e1bdf1feec11/posthog-core/src/index.ts#L145
Love that they completely rewrote in a minor version
There was a problem hiding this comment.
This would work for a single user's machine, but will create spam in recurring CI pipelines which is why I had to override in the first place.
There was a problem hiding this comment.
This would work for a single user's machine, but will create spam in recurring CI pipelines which is why I had to override in the first place.
Totally forgot about this use case, but should we instead just force disable for CI? This flow works correctly for individual developer usage whereas the other one didn't.
There was a problem hiding this comment.
I'd be down to force disable for CI. Prob not the analytics you're looking for. We have the is-ci pkg
There was a problem hiding this comment.
I'd be down to force disable for CI. Prob not the analytics you're looking for. We have the is-ci pkg
Added now
There was a problem hiding this comment.
I'm not sure why posthog-node doesn't inherit from this class, but this is what the web and react packages extend so there's easy getDistinctId() methods which falls back to the anon ID if there's nothing else there:
https://github.com/PostHog/posthog-js-lite/blob/master/posthog-core/src/index.ts#L606
There was a problem hiding this comment.
The docs and marketing sites don't use distinct id at all right? So we shouldn't be using it here when its not the distinct id. What do you think of my proposal for captureStateless?
There was a problem hiding this comment.
Yeah so we def want to use the anon id. It seems like an easy change and then we don't need to worry about disabling CI. Although up to you if you still want to disable. We could add a tag for isCI so we could filter out that usage.
There was a problem hiding this comment.
@mueschm looping you in here per @tjhiggins request. He has to take off and said that you can take over reviewing and merging as you see fit.
We spoke offline as well and concluded that the implementation in this PR seems correct. See this docs article which took us a while to find that confirms how the aliasing works:
https://posthog.com/docs/integrate/user-properties#how-are-properties-managed-when-merging-or-aliasing-users
| @@ -47,7 +47,7 @@ | |||
| "opener": "^1.5.2", | |||
| "p-limit": "^3.1.0", | |||
| "patch-package": "^6.4.7", | |||
| "posthog-node": "^2.4.0", | |||
| "posthog-node": "^2.5.1", | |||
There was a problem hiding this comment.
This pkg requires node 15+which is kind of weird. I guess we have the ci tests for node 14.
There was a problem hiding this comment.
That's odd. The tests seem to have passed on node 14 and npm didn't alert me of the issue. How did you find out about the requirement?
There was a problem hiding this comment.
Was looking at the package-lock.json trying to figure out why my previous patch didn't work. Thought maybe it was using 2.5+ in the package-lock which would explain why the override wasn't working anymore.
src/app-config/auth.ts
Outdated
| @@ -18,24 +18,35 @@ interface AuthResult { | |||
| id_token?: string; | |||
| } | |||
|
|
|||
| export type AuthClientOptions = { | |||
| checkLogin: () => Promise<User>; | |||
There was a problem hiding this comment.
So with the last minute realization on Friday. Do we need the checkLogin callbacks? Your initial logic which called alias in the checkLogin seemingly would have been sufficient.
There was a problem hiding this comment.
hmmm, there's definitely some room for re-working, but we can't just outright get rid of it because we use it to acquire the User object here:
https://github.com/architect-team/architect-cli/blob/bugfix/621/src/app-config/auth.ts#L166
Though there's something weirdly circular about the current implementation of the two callbacks now that you point it out.
There was a problem hiding this comment.
@tjhiggins updated this. Feel free to review and then let's get it merged in so we capture events correctly.
## [1.33.2-rc.1](v1.33.1...v1.33.2-rc.1) (2023-02-22) ### Bug Fixes * **analytics:** Fix issue with incorrect distinct id ([#834](#834)) ([926902e](926902e))
|
🎉 This PR is included in version 1.33.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Overview
We had some issues attributing events to individual sessions making it hard to measure performance of our longer running commands.
Changes
Added tracking event to capture command start time, and updated capture method to use anonymous id.