kedro-org / kedro-viz

Visualise your Kedro data and machine-learning pipelines and track your experiments.
https://demo.kedro.org
Apache License 2.0
669 stars 110 forks source link

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

Open DimedS opened 1 month ago

DimedS commented 1 month ago

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

astrojuanlu commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

Thanks. Linking the code for reference

https://github.com/kedro-org/kedro-viz/blob/e719f303494a44fcb7040797c45c4c22d8d7142c/package/kedro_viz/integrations/kedro/telemetry.py#L18-L31

astrojuanlu commented 1 month ago

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 commented 1 month ago

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.