Identify telemetry events when user identify is known#860
Identify telemetry events when user identify is known#860
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
There was a problem hiding this comment.
Code Review
This pull request replaces the existing telemetry implementation with a new system in src/tabpfn/telemetry.py and adds pyjwt as a dependency. The review identifies several performance and architectural concerns: the _patch_client function configures a local instance that is not persisted, and ProductTelemetry is redundantly instantiated on every event capture. Furthermore, the Posthog client should utilize the api_host from the configuration, and the decoded user ID should be cached to prevent repeated JWT decoding overhead.
dianaprior
left a comment
There was a problem hiding this comment.
Some important comments re: correctness of telemetry file
brendan-priorlabs
left a comment
There was a problem hiding this comment.
Thanks for the PR, Dominik! Code seems clean. Left some small comments. Thank you @dianaprior as well for covering the README side of the review!
|
Oh and for the test failures, |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Dominik Safaric <dominik@sulie.co>
…ENG-566/telemetry
brendan-priorlabs
left a comment
There was a problem hiding this comment.
LGTM on the code side. Thanks, Dominik!
brendan-priorlabs
left a comment
There was a problem hiding this comment.
Sorry, withdrawing LGTM pending internal discussion :/
Issue
With this PR, and only for identified usage - where users have accepted the license and created a Prior Labs account - we begin identifying telemetry events.
For anonymous events, we also drop recording
fit_calledandpredict_calledevents.Motivation and Context
Motivated by the recent gating changes where we switched from HuggingFace to model gating via ux.priorlabs.ai.
Public API Changes
How Has This Been Tested?
Yes, manually and also with unit tests.
Checklist
changelog/README.md), or "no changelog needed" label requested.