feat(telemetry): Unique User IDs in kedro-telemetry - merge only for kedro-telemetry release 0.4.0 #596

DimedS · 2024-03-05T13:03:08Z

Description

This PR introduces a unique identifier (UUID) for each user of the kedro-telemetry.

Instead of relying on hashed usernames, we now generate a unique UUID for each user. This UUID is generated using uuid.uuid4() function.
Generated UUID is stored in a user-specific configuration file, leveraging the appdirs library to determine the appropriate OS-specific path for this file (~/Library/Application Support/kedro/telemetry.conf on MAC)
If a UUID does not already exist, the function generates a new one, stores it, and then uses this UUID in subsequent telemetry events.

Checklist

Opened this PR as a 'Draft Pull Request' if it is work-in-progress
Updated the documentation to reflect the code changes
Added a description of this change in the relevant RELEASE.md file
Added tests to cover my changes

Signed-off-by: Dmitry Sorokin <dmd40in@gmail.com>

Signed-off-by: lrcouto <laurarccouto@gmail.com>

lrcouto

Did some small changes to the structure of the function to leave only the part that actually handles file operations inside the try block.

Signed-off-by: lrcouto <laurarccouto@gmail.com>

noklam · 2024-03-06T17:22:28Z

kedro-telemetry/kedro_telemetry/plugin.py

@@ -15,6 +16,7 @@
 import requests
 import toml
 import yaml
+from appdirs import user_config_dir


is this a new dependency?

is this a new dependency?

yes, we added it to the dependencies list

noklam · 2024-03-06T17:22:56Z

kedro-telemetry/kedro_telemetry/plugin.py

@@ -41,6 +43,7 @@
    "BUILDKITE",  # https://buildkite.com/docs/pipelines/environment-variables
 }
 TIMESTAMP_FORMAT = "%Y-%m-%dT%H:%M:%S.%fZ"
+CONFIG_FILENAME = "telemetry.conf"


is it expected to be commited into the repo?

is it expected to be commited into the repo?

You mean this file? It will be stored in the user's configuration directory, not in project directory, so looks like it will not be possible to commit it.

noklam · 2024-03-06T19:09:39Z

kedro-telemetry/kedro_telemetry/plugin.py

+def _get_or_create_uuid():
+    """
+    Reads a UUID from a configuration file or generates and saves a new one if not present.
+    """
+    config_path = user_config_dir("kedro")
+    full_path = os.path.join(config_path, CONFIG_FILENAME)
+    config = ConfigParser()
+
    try:
-        username = getpass.getuser()
-        return _hash(username)
-    except Exception as exc:
-        logger.warning(
-            "Something went wrong with getting the username. Exception: %s",
-            exc,
-        )
+        if os.path.exists(full_path):
+            config.read(full_path)
+
+            if config.has_section("telemetry") and "uuid" in config["telemetry"]:
+                try:
+                    return uuid.UUID(config["telemetry"]["uuid"]).hex
+                except ValueError:
+                    pass  # Invalid UUID found, will generate a new one
+
+        # Generate a new UUID and save it to the config file
+        if not config.has_section("telemetry"):
+            config.add_section("telemetry")
+        new_uuid = uuid.uuid4().hex
+        config.set("telemetry", "uuid", new_uuid)
+
+        os.makedirs(config_path, exist_ok=True)
+        with open(full_path, "w") as configfile:
+            config.write(configfile)
+
+        return new_uuid
+
+    except Exception as e:
+        logging.error(f"Failed to get or create UUID: {e}")


Can this be splitted into two function? doing both read and write in a function is a bit too much and hard to test.

For example, how are you testing for the case when config exist but telemetry section does not exist?

astrojuanlu

Left a few comments, thanks @DimedS and @lrcouto !

kedro-telemetry/RELEASE.md

kedro-telemetry/kedro_telemetry/plugin.py

Signed-off-by: lrcouto <laurarccouto@gmail.com>

DimedS · 2024-03-08T14:48:58Z

kedro-telemetry/kedro_telemetry/plugin.py

+                config = toml.load(f)
+
+                if "telemetry" in config and "uuid" in config["telemetry"]:
+                    try:


looks like we don't need additional try more, because we are returning the same for any error on level upper

That's true, I'm gonna change it

Signed-off-by: lrcouto <laurarccouto@gmail.com>

merelcht

I left some minor comments, but otherwise looks good! 👍 Also tested locally and seems like it's all working.

merelcht · 2024-03-08T15:15:21Z

kedro-telemetry/kedro_telemetry/plugin.py

+        return ""
+
+
+def _generate_new_uuid(full_path):


I just noticed we're not doing any typing in kedro-telemetry, but can we maybe start adding that for new functions like this? 🙂

merelcht · 2024-03-08T15:17:14Z

kedro-telemetry/kedro_telemetry/plugin.py

@@ -90,7 +122,7 @@ def before_command_run(
            main_command = masked_command_args[0] if masked_command_args else "kedro"

            logger.debug("You have opted into product usage analytics.")
-            hashed_username = _get_hashed_username()
+            hashed_username = _get_or_create_uuid()


Should the variable be renamed to user_uuid or something like that now it's not a hashed username anymore?

merelcht · 2024-03-08T15:18:13Z

kedro-telemetry/tests/test_plugin.py


        mocked_heap_call = mocker.patch("kedro_telemetry.plugin._send_heap_event")
        telemetry_hook = KedroTelemetryCLIHooks()
        command_args = []
        telemetry_hook.before_command_run(fake_metadata, command_args)
        expected_properties = {
-            "username": "digested",
+            "username": "hashed_username",


NIT: This is the uuid now right? I know it's just test data, but maybe for clarity it would be helpful to specify the actual data so uuid in this case.

Signed-off-by: lrcouto <laurarccouto@gmail.com>

ankatiyar

Left some comments but looks good to me! Thanks @lrcouto and @DimedS!

ankatiyar · 2024-03-12T10:23:58Z

kedro-telemetry/RELEASE.md

@@ -1,4 +1,5 @@
 # Upcoming release
+* Updated the plugin to generate an unique UUID for each user of `kedro-telemetry`. (On release 0.4)


Suggested change

* Updated the plugin to generate an unique UUID for each user of `kedro-telemetry`. (On release 0.4)

* Updated the plugin to generate an unique UUID for each user of `kedro-telemetry`.

The heading will say the release number anyway

ankatiyar · 2024-03-12T10:31:00Z

kedro-telemetry/kedro_telemetry/plugin.py

@@ -90,17 +122,17 @@ def before_command_run(
            main_command = masked_command_args[0] if masked_command_args else "kedro"


I can't comment on the line above since it hasn't been changed but could line 117 be changed to -
cli = KedroCLI(project_path=metadata.project_path)
Following the changes in kedro-org/kedro#3683, I think this will allow us to capture commands called from within subdirectories also and mask them properly

Signed-off-by: lrcouto <laurarccouto@gmail.com>

merelcht

LGTM! ⭐

astrojuanlu · 2024-03-18T20:54:27Z

Tested this and tried to break it in various ways, didn't find anything weird ⭐ Thanks @DimedS and @lrcouto!

Change to unique uuid

2b36ddd

Signed-off-by: Dmitry Sorokin <dmd40in@gmail.com>

DimedS linked an issue Mar 5, 2024 that may be closed by this pull request

User identity for telemetry events is not unique #333

Closed

DimedS requested a review from lrcouto March 5, 2024 13:04

Fix tests

2735206

Signed-off-by: Dmitry Sorokin <dmd40in@gmail.com>

DimedS changed the title ~~Unique User IDs in kedro-telemetry~~ fix: Unique User IDs in kedro-telemetry Mar 5, 2024

DimedS changed the title ~~fix: Unique User IDs in kedro-telemetry~~ telemetry: Unique User IDs in kedro-telemetry Mar 5, 2024

DimedS changed the title ~~telemetry: Unique User IDs in kedro-telemetry~~ feat(telemetry): Unique User IDs in kedro-telemetry Mar 5, 2024

fix tests and empty value

a17da4a

Signed-off-by: Dmitry Sorokin <dmd40in@gmail.com>

DimedS force-pushed the unique-user-is-telemetry branch from c75b14e to a17da4a Compare March 5, 2024 17:27

DimedS and others added 4 commits March 5, 2024 17:33

fix lint

5c1f2be

Signed-off-by: Dmitry Sorokin <dmd40in@gmail.com>

Reformat _get_or_create_uuid to not be entirely inside the try block

75480b6

Signed-off-by: lrcouto <laurarccouto@gmail.com>

Reformat _get_or_create_uuid to not be entirely inside the try block

11c28b1

Signed-off-by: lrcouto <laurarccouto@gmail.com>

Remove incorrectly sent file

4c9427c

Signed-off-by: lrcouto <laurarccouto@gmail.com>

lrcouto marked this pull request as ready for review March 6, 2024 16:28

lrcouto reviewed Mar 6, 2024

View reviewed changes

lrcouto requested review from astrojuanlu, noklam, merelcht and ankatiyar March 6, 2024 16:29

Update RELEASE.md

a20d0d8

Signed-off-by: lrcouto <laurarccouto@gmail.com>

noklam reviewed Mar 6, 2024

View reviewed changes

astrojuanlu reviewed Mar 7, 2024

View reviewed changes

kedro-telemetry/RELEASE.md Show resolved Hide resolved

kedro-telemetry/kedro_telemetry/plugin.py Show resolved Hide resolved

kedro-telemetry/kedro_telemetry/plugin.py Outdated Show resolved Hide resolved

kedro-telemetry/kedro_telemetry/plugin.py Outdated Show resolved Hide resolved

lrcouto added 4 commits March 7, 2024 12:39

Return empty ID if the existing ID cannot be read

ea96a13

Signed-off-by: lrcouto <laurarccouto@gmail.com>

Extract new uuid generation to its own function

e16cde6

Signed-off-by: lrcouto <laurarccouto@gmail.com>

Convert .conf file to .toml

ec0594e

Signed-off-by: lrcouto <laurarccouto@gmail.com>

Lint

3bc7546

Signed-off-by: lrcouto <laurarccouto@gmail.com>

DimedS commented Mar 8, 2024

View reviewed changes

Remove redundant try/except block

9795b5e

Signed-off-by: lrcouto <laurarccouto@gmail.com>

merelcht reviewed Mar 8, 2024

View reviewed changes

Add type hints, change variable names to be more descriptive

ae5694a

Signed-off-by: lrcouto <laurarccouto@gmail.com>

ankatiyar approved these changes Mar 12, 2024

View reviewed changes

Small fixes

0c706f0

Signed-off-by: lrcouto <laurarccouto@gmail.com>

lrcouto changed the title ~~feat(telemetry): Unique User IDs in kedro-telemetry~~ feat(telemetry): Unique User IDs in kedro-telemetry - merge only for kedro-telemetry release 0.4.0 Mar 13, 2024

Make release note more explicit

b1bca73

Signed-off-by: lrcouto <laurarccouto@gmail.com>

merelcht approved these changes Mar 13, 2024

View reviewed changes

astrojuanlu self-requested a review March 13, 2024 16:12

astrojuanlu force-pushed the unique-user-is-telemetry branch from df95d34 to b1bca73 Compare March 14, 2024 10:15

lrcouto added 2 commits March 15, 2024 12:04

Merge branch 'main' into unique-user-is-telemetry

74cf22d

Merge branch 'main' into unique-user-is-telemetry

313f448

lrcouto merged commit 846db2b into main Mar 18, 2024
13 checks passed

lrcouto deleted the unique-user-is-telemetry branch March 18, 2024 14:32

DimedS mentioned this pull request Apr 11, 2024

Update UUID telemetry docs kedro-org/kedro#3805

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(telemetry): Unique User IDs in kedro-telemetry - merge only for kedro-telemetry release 0.4.0 #596

feat(telemetry): Unique User IDs in kedro-telemetry - merge only for kedro-telemetry release 0.4.0 #596

DimedS commented Mar 5, 2024 •

edited by lrcouto

lrcouto left a comment

noklam Mar 6, 2024

DimedS Mar 6, 2024

noklam Mar 6, 2024

DimedS Mar 6, 2024

noklam Mar 6, 2024

astrojuanlu left a comment

DimedS Mar 8, 2024

lrcouto Mar 8, 2024

merelcht left a comment

merelcht Mar 8, 2024

merelcht Mar 8, 2024

merelcht Mar 8, 2024

ankatiyar left a comment

ankatiyar Mar 12, 2024

ankatiyar Mar 12, 2024

ankatiyar Mar 12, 2024

merelcht left a comment

astrojuanlu commented Mar 18, 2024

		@@ -1,4 +1,5 @@
		# Upcoming release
		* Updated the plugin to generate an unique UUID for each user of `kedro-telemetry`. (On release 0.4)

		@@ -90,17 +122,17 @@ def before_command_run(
		main_command = masked_command_args[0] if masked_command_args else "kedro"

feat(telemetry): Unique User IDs in kedro-telemetry - merge only for kedro-telemetry release 0.4.0 #596

feat(telemetry): Unique User IDs in kedro-telemetry - merge only for kedro-telemetry release 0.4.0 #596

Conversation

DimedS commented Mar 5, 2024 • edited by lrcouto

Description

Checklist

lrcouto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astrojuanlu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merelcht left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ankatiyar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merelcht left a comment

Choose a reason for hiding this comment

astrojuanlu commented Mar 18, 2024

DimedS commented Mar 5, 2024 •

edited by lrcouto