Skip to content

[On hold] Identify telemetry events when user identify is known#860

Closed
safaricd wants to merge 6 commits into
mainfrom
ENG-566/telemetry
Closed

[On hold] Identify telemetry events when user identify is known#860
safaricd wants to merge 6 commits into
mainfrom
ENG-566/telemetry

Conversation

@safaricd

@safaricd safaricd commented Apr 3, 2026

Copy link
Copy Markdown
Collaborator

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

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

Copy link
Copy Markdown
Contributor

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

@dianaprior dianaprior left a comment

Copy link
Copy Markdown

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

@brendan-priorlabs brendan-priorlabs left a comment

Copy link
Copy Markdown
Contributor

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>

@brendan-priorlabs brendan-priorlabs left a comment

Copy link
Copy Markdown
Contributor

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!

@brendan-priorlabs brendan-priorlabs left a comment

Copy link
Copy Markdown
Contributor

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 :/

@noahho

noahho commented May 16, 2026

Copy link
Copy Markdown
Collaborator

@safaricd could you take a look at this when you get a chance?

@noahho noahho changed the title Identify telemetry events when user identify is known [On hold] Identify telemetry events when user identify is known Jun 19, 2026
@klemens-floege

Copy link
Copy Markdown
Contributor

@safaricd can we close this PR or are you still working on it

@dianaprior

Copy link
Copy Markdown

@safaricd can we close this PR or are you still working on it

not Dominik, but we should 100% close this one - we are not doing OSS telemetry anytime soon

@klemens-floege

Copy link
Copy Markdown
Contributor

@dianaprior thanks! I will close this PR now; @safaricd feel free to re-open if necessary :)

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.

5 participants