Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update kedro-viz telemetry to reflect recent changes in kedro-telemetry #2020

Closed
1 task
DimedS opened this issue Aug 1, 2024 · 8 comments · Fixed by #2112
Closed
1 task

Update kedro-viz telemetry to reflect recent changes in kedro-telemetry #2020

DimedS opened this issue Aug 1, 2024 · 8 comments · Fixed by #2112

Comments

@DimedS
Copy link

DimedS commented Aug 1, 2024

Description

We switched to an opt-out model in Kedro-Telemetry as described in issue #715. This change introduces new methods for obtaining telemetry consent, which should be incorporated into the Viz telemetry. Additionally, a new way to establish the user ID was introduced in PR #596, and this should also be updated in Viz.

Context

Why is this change important to you? How would you use it? How can it benefit other users?

Possible Implementation

(Optional) Suggest an idea for implementing the addition or change.

Possible Alternatives

(Optional) Describe any alternative solutions or features you've considered.

Checklist

  • Include labels so that we can categorise your feature request
@astrojuanlu
Copy link
Member

Thanks for opening this @DimedS. Help me understand: what would be the user-facing consequences of releasing Kedro without making these changes in Kedro-Viz?

@DimedS
Copy link
Author

DimedS commented Aug 1, 2024

Based on the Viz code, if users disable telemetry using new environment variables but still have a .telemetry file with consent: True in their project folder, Kedro-Viz will continue to send telemetry data. In other scenarios, the behavior is as expected:

def get_heap_app_id(project_path: Path) -> Optional[str]:
    """Return the Heap App ID used for Kedro telemetry if user has given consent."""
    if not _IS_TELEMETRY_INSTALLED:  # pragma: no cover
        return None
    telemetry_file_path = project_path / ".telemetry"
    if not telemetry_file_path.exists():
        return None
    with open(
        telemetry_file_path, encoding="utf8"
    ) as telemetry_file:  # pylint: disable: unspecified-encoding
        telemetry = yaml.safe_load(telemetry_file)
        if _is_valid_syntax(telemetry) and telemetry["consent"]:
            return _get_heap_app_id()
    return None

@astrojuanlu
Copy link
Member

Thanks. Linking the code for reference

def get_heap_app_id(project_path: Path) -> Optional[str]:
"""Return the Heap App ID used for Kedro telemetry if user has given consent."""
if not _IS_TELEMETRY_INSTALLED: # pragma: no cover
return None
telemetry_file_path = project_path / ".telemetry"
if not telemetry_file_path.exists():
return None
with open(
telemetry_file_path, encoding="utf8"
) as telemetry_file: # pylint: disable: unspecified-encoding
telemetry = yaml.safe_load(telemetry_file)
if _is_valid_syntax(telemetry) and telemetry["consent"]:
return _get_heap_app_id()
return None

@astrojuanlu
Copy link
Member

Perhaps kedro-telemetry should have this logic so that Kedro-Viz can import it? (That would make kedro-telemetry a dependency of kedro-viz, which already is, transitively through kedro)

@DimedS
Copy link
Author

DimedS commented Aug 1, 2024

Perhaps kedro-telemetry should have this logic so that Kedro-Viz can import it? (That would make kedro-telemetry a dependency of kedro-viz, which already is, transitively through kedro)

I agree; it makes sense to depend on kedro-telemetry for handling the consent check, which is encapsulated in the _check_for_telemetry_consent() function. This function could be beneficial for kedro-viz. However, given that the code is currently small and straightforward, it might be wise to wait until telemetry is fully integrated into Kedro before making that changes with dependencies.

@DimedS
Copy link
Author

DimedS commented Sep 24, 2024

Discussed with @jitu5, it makes sense to import the following functions from kedro-telemetry to viz:

  • _check_for_telemetry_consent() - to check for consent
  • _get_or_create_uuid() - to retrieve the same UUID used by telemetry

However, the user can manually remove kedro-telemetry even if it was previously installed with Kedro. Therefore, the import should be safe, and if kedro-telemetry is not installed, it should be treated as consent being False.

@jitu5
Copy link
Contributor

jitu5 commented Sep 24, 2024

Discussed with @jitu5, it makes sense to import the following functions from kedro-telemetry to viz:

  • _check_for_telemetry_consent() - to check for consent
  • _get_or_create_uuid() - to retrieve the same UUID used by telemetry

However, the user can manually remove kedro-telemetry even if it was previously installed with Kedro. Therefore, the import should be safe, and if kedro-telemetry is not installed, it should be treated as consent being False.

Make sense to reuse what we already have, @astrojuanlu @rashidakanchwala wdyt ?

@astrojuanlu
Copy link
Member

if kedro-telemetry is not installed, it should be treated as consent being False.

👍🏼

About reusing the code or not, I leave it in your hands!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants