[MAINTENANCE] Add wrapper for posthog analytics as well as an initial event #8977
Conversation
✅ Deploy Preview for niobium-lead-7998 canceled.
|
wookasz
force-pushed
the
m/v1-28/add-analytics
branch
from
75c5b08 to
4461abf
Compare
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Left some thoughts on the event.action stuff.
I think we should get some basic testing in place on this - does initializing the posthog config actually work? Does submit capture what it looks like we capture? But I won't block the merge on that.
Seems like this should be fairly ergonomic!
|
|
||
|
|
||
| class Config(GenericModel): | ||
| organization_id: UUID = UUID("00000000-0000-0000-0000-000000000000") |
There was a problem hiding this comment.
Is a fake UUID better than None? If so, should/could we make it a named constant e.g. DUMMY_UUID
There was a problem hiding this comment.
Yes, Tetris preferred dummies in the past
| def __post_init__(self): | ||
| allowed_actions = self.get_allowed_actions() | ||
| if ( | ||
| allowed_actions is not None |
There was a problem hiding this comment.
I feel like this protection isn't really protecting us from much; subclasses have to opt in to using it, and then the subclass defines both its action and that the action is allowed. Maybe there are cases I'm missing though.
There was a problem hiding this comment.
@tyler-hoffman in the case of this specific Event, there was only one Action that can be associated with it and to make using the event a bit easier, I set it in the ctor. However, there's another, more general pattern here. For example, a CheckpointEvent may support multiple actions, e.g. checkpoint.created, checkpoint.updated, checkpoint.deleted.
The use of allowed_actions become clearer once we add code to generate an event schema file for use in our analytics pipelines.
| _allowed_actions: ClassVar[List[Action]] = [DATA_CONTEXT_INITIALIZED] | ||
|
|
||
| def __init__(self): | ||
| super().__init__(action=self.action) |
There was a problem hiding this comment.
The flow here feels off to me. This and the parent class both define an action field, and we read this during init and pass it to the parent, which under the hood assigns it to self.action again. I would remove line 8 and just call super().__init__(action=DATA_CONTEXT_INITIALIZED)
github-merge-queue
bot
removed this pull request from the merge queue due to no response for status checks
github-merge-queue
bot
removed this pull request from the merge queue due to no response for status checks
Add basic analytics framework and use it in a single "data_context.initialized" event
invoke lint(usesblack+ruff)For more information about contributing, see Contribute.
After you submit your PR, keep the page open and monitor the statuses of the various checks made by our continuous integration process at the bottom of the page. Please fix any issues that come up and reach out on Slack if you need help. Thanks for contributing!