Rework telemetry to support installation_id #2480
Conversation
reflex/reflex.py
Outdated
| @@ -80,6 +81,10 @@ def _init( | |||
|
|
|||
| prerequisites.check_latest_package_version(constants.Reflex.MODULE_NAME) | |||
|
|
|||
| prerequisites.initialize_reflex_user_directory() | |||
|
|
|||
| ensure_reflex_distinct_id() | |||
There was a problem hiding this comment.
Can this just go inside of initialize_reflex_user_directory to keep everything in the prerequisites file?
There was a problem hiding this comment.
It is in the prerequisite file. Removed the separate import. It will say prerequisites.ensure_reflex_distinct_id() now.
reflex/utils/prerequisites.py
Outdated
| """Ensures that a reflex distinct id has been generated and stored in the reflex directory. | ||
|
|
||
| Returns: | ||
| Distinct id (int). |
There was a problem hiding this comment.
nit: don't need to annotate return types since we use annotations
reflex/utils/prerequisites.py
Outdated
| Distinct id (int). | ||
| """ | ||
| initialize_reflex_user_directory() | ||
| distinct_id_file = os.path.join(constants.Reflex.DIR, "distinct_id") |
There was a problem hiding this comment.
Maybe call the file reflex.json to match how we do it in the .web folder, we can reuse the file parsing/writing functions for it too then
There was a problem hiding this comment.
There isn't really any existing logic, it's all raw json.loads into dictionary, etc.
The reason I was leaning to a flat, single purpose file is for simplicity. Without any further things to be stashed, it would be better to keep it minimal.
We can discuss the naming further - we can finalize a name on Discord and just update it to that.
jackie-pc
changed the title
reflex/utils/prerequisites.py
Outdated
| pass | ||
|
|
||
| if installation_id is None: | ||
| installation_id = random.getrandbits(128) |
There was a problem hiding this comment.
Should we use uuid4 instead of random bits in both places?
There was a problem hiding this comment.
I went with what was there already. It seems acceptable for our purpose. What would drive us to change it to uuid?
| @@ -80,6 +80,10 @@ def _init( | |||
|
|
|||
| prerequisites.check_latest_package_version(constants.Reflex.MODULE_NAME) | |||
|
|
|||
| prerequisites.initialize_reflex_user_directory() | |||
|
|
|||
| prerequisites.ensure_reflex_installation_id() | |||
There was a problem hiding this comment.
should we do this, even if the user has disabled telemetry? my thought is TELEMETRY_DISABLED=1 and we don't generate a marker file at all
There was a problem hiding this comment.
Currently, project_hash has exactly the same nature - it is only used for telemetry, and yet we always generate it even if TELEMETRY_DISABLED=1. I.e. we persist something, but we just never send out.
Checking for the flag costs some incremental complexity. Note even if we did do it, it would not be clean. Suppose telemetry was only disabled after initial use. The file would have already been created.
Don't really feel strongly either way about it - if others do, of course we can just add the check!
There was a problem hiding this comment.
I think we can keep the file, as long as we never send the data when the flag is enabled
There was a problem hiding this comment.
Since ensure_reflex_installation_id already calls initialize_reflex_user_directory() do we need both calls? that's what I was thinking originally - just calling ensure_reflex_installation_id at the end of initialize_ every time
reflex/utils/prerequisites.py
Outdated
| with open(installation_id_file, "w") as f: | ||
| f.write(str(installation_id)) |
There was a problem hiding this comment.
If we cannot write out the file for some reason, i don't think reflex should just raise an exception and exit, that seems like a potentially poor UX
There was a problem hiding this comment.
AGREE that this is not supposed to be a UX at all.
From this viewpoint, the most we should do is perhaps catch exception and add a console.log.debug.
| @@ -80,6 +80,10 @@ def _init( | |||
|
|
|||
| prerequisites.check_latest_package_version(constants.Reflex.MODULE_NAME) | |||
|
|
|||
| prerequisites.initialize_reflex_user_directory() | |||
|
|
|||
| prerequisites.ensure_reflex_installation_id() | |||
There was a problem hiding this comment.
Since ensure_reflex_installation_id already calls initialize_reflex_user_directory() do we need both calls? that's what I was thinking originally - just calling ensure_reflex_installation_id at the end of initialize_ every time
No description provided.