Skip to content

Identify telemetry events when user identify is known#860

Open
safaricd wants to merge 6 commits intomainfrom
ENG-566/telemetry
Open

Identify telemetry events when user identify is known#860
safaricd wants to merge 6 commits intomainfrom
ENG-566/telemetry

Conversation

@safaricd
Copy link
Copy Markdown
Collaborator

@safaricd safaricd commented Apr 3, 2026

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_called and predict_called events.

Motivation and Context

Motivated by the recent gating changes where we switched from HuggingFace to model gating via ux.priorlabs.ai.

Public API Changes

  • No Public API changes

How Has This Been Tested?

Yes, manually and also with unit tests.

Checklist

  • The changes have been tested locally.
  • Documentation has been updated (if the public API or usage changes).
  • A changelog entry has been added (see changelog/README.md), or "no changelog needed" label requested.
  • The code follows the project's style guidelines.
  • I have considered the impact of these changes on the public API.

@safaricd safaricd requested a review from a team as a code owner April 3, 2026 11:48
@safaricd safaricd requested review from klemens-floege and removed request for a team April 3, 2026 11:48
@chatgpt-codex-connector
Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@safaricd safaricd requested review from brendan-priorlabs and simo-prior and removed request for klemens-floege April 3, 2026 11:49
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

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.

Comment thread src/tabpfn/telemetry.py
Comment thread src/tabpfn/telemetry.py
Comment thread src/tabpfn/telemetry.py Outdated
Comment thread src/tabpfn/telemetry.py
Copy link
Copy Markdown

@dianaprior dianaprior left a comment

Choose a reason for hiding this comment

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

Some important comments re: correctness of telemetry file

Comment thread TELEMETRY.md Outdated
Comment thread TELEMETRY.md Outdated
Comment thread TELEMETRY.md Outdated
Comment thread TELEMETRY.md Outdated
Comment thread TELEMETRY.md Outdated
Copy link
Copy Markdown
Contributor

@brendan-priorlabs brendan-priorlabs left a comment

Choose a reason for hiding this comment

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

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!

Comment thread src/tabpfn/telemetry.py Outdated
Comment thread TELEMETRY.md Outdated
@brendan-priorlabs
Copy link
Copy Markdown
Contributor

Oh and for the test failures, ImportError: cannot import name 'ModelLoadEvent' from 'tabpfn_common_utils.telemetry.core.events, do we just need a version bump on tabpfn_common_utils?

safaricd and others added 4 commits April 8, 2026 15:56
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: Dominik Safaric <dominik@sulie.co>
Copy link
Copy Markdown
Contributor

@brendan-priorlabs brendan-priorlabs left a comment

Choose a reason for hiding this comment

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

LGTM on the code side. Thanks, Dominik!

Copy link
Copy Markdown
Contributor

@brendan-priorlabs brendan-priorlabs left a comment

Choose a reason for hiding this comment

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

Sorry, withdrawing LGTM pending internal discussion :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants