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

feat(telemetry): Remove consent confirmation prompt for kedro-telemetry #744

Merged
merged 11 commits into from
Jul 5, 2024

Conversation

ElenaKhaustova
Copy link
Contributor

@ElenaKhaustova ElenaKhaustova commented Jul 1, 2024

Description

Partly solves #726

Development notes

  • _confirm_consent was removed
  • now we use consent from the .telemetry file if it exists and has a valid format otherwise, we consider consent is True

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: Elena Khaustova <[email protected]>
Signed-off-by: Elena Khaustova <[email protected]>
Signed-off-by: Elena Khaustova <[email protected]>
@ElenaKhaustova ElenaKhaustova changed the title Remove consent confirmation prompt for kedro-telemetry feat(telemetry): remove consent confirmation prompt for kedro-telemetry Jul 1, 2024
@ElenaKhaustova ElenaKhaustova changed the title feat(telemetry): remove consent confirmation prompt for kedro-telemetry feat(telemetry): Remove consent confirmation prompt for kedro-telemetry Jul 1, 2024
@ElenaKhaustova ElenaKhaustova marked this pull request as ready for review July 1, 2024 15:21
@astrojuanlu
Copy link
Member

A few things:

$ kedro run
[07/04/24 08:19:51] INFO     Using `conf/logging.yml` as logging configuration. You can change this by setting the KEDRO_LOGGING_CONFIG environment __init__.py:249
                             variable accordingly.                                                                                                                 
                    ERROR    Failed to retrieve UUID: badly formed hexadecimal UUID string                                                             plugin.py:78
[07/04/24 08:19:52] WARNING  Failed to send data to Heap. Response code returned: 400, Response reason: Bad Request                                   plugin.py:336
                    WARNING  Failed to send data to Heap. Response code returned: 400, Response reason: Bad Request                                   plugin.py:336

which is probably not related to this PR, but maybe it's too scary (specially given that it's printed twice, maybe because of #730?). I think there are 2 things here:

And finally, the "Kedro is sending anonymous usage data" message I proposed in #715 is not being shown.

@ElenaKhaustova
Copy link
Contributor Author

ElenaKhaustova commented Jul 4, 2024

A few things:

$ kedro run
[07/04/24 08:19:51] INFO     Using `conf/logging.yml` as logging configuration. You can change this by setting the KEDRO_LOGGING_CONFIG environment __init__.py:249
                             variable accordingly.                                                                                                                 
                    ERROR    Failed to retrieve UUID: badly formed hexadecimal UUID string                                                             plugin.py:78
[07/04/24 08:19:52] WARNING  Failed to send data to Heap. Response code returned: 400, Response reason: Bad Request                                   plugin.py:336
                    WARNING  Failed to send data to Heap. Response code returned: 400, Response reason: Bad Request                                   plugin.py:336

which is probably not related to this PR, but maybe it's too scary (specially given that it's printed twice, maybe because of #730?). I think there are 2 things here:

And finally, the "Kedro is sending anonymous usage data" message I proposed in #715 is not being shown.

@astrojuanlu, thank you for the review!

  1. I believe there is not much difference if we merge this PR before #3976 or after as it only removes the consent confirmation prompt and works fine with the latest kedro==0.19.6 version and changes introduced in the PR #3976
  2. I think we can follow @DimedS split and address kedro-telemetry: Implement telemetry message notification #727 ("Kedro is sending anonymous usage data" will be added with this PR) and kedro-telemetry: Introduce environment variables for disabling telemetry and revise disabling methods #728 separately on top of this PR and, once they're ready, merge them together one by one.
  3. I was not able to reproduce the errors you mentioned. I've tested the current change on spaceflights-pandas with kedro==0.19.6 and changes from here #3976. Both work well. I double checked the response status when sending to the heap - it looks good - 200 OK. I also checked that project ID is creating well and is part of the query. But anyway, the current changes cannot not affect the project ID logic. Could you please check if the error is not appearing for you without current changes?

Copy link
Contributor

@DimedS DimedS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @ElenaKhaustova. I tested all the PRs together, and everything works well. I also checked the telemetry info on Heap, and it's functioning correctly.

I believe we can merge the current PR and this one, then continue with message notification and new disabling methods, followed by other telemetry tickets.

I agree on merging the Starters PR last, just before the release.

Copy link
Member

@merelcht merelcht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't test this, but from a code perspective this looks good to me. I agree that the environment variables should be done separately. We just need to make sure we don't do any releases until the full opt-out flow is implemented in all parts.

@@ -347,14 +346,17 @@ def _send_heap_event(


def _check_for_telemetry_consent(project_path: Path) -> bool:
"""
Use a telemetry consent from ".telemetry" file if it exists and has a valid format.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Use a telemetry consent from ".telemetry" file if it exists and has a valid format.
Use telemetry consent from ".telemetry" file if it exists and has a valid format.

Signed-off-by: Elena Khaustova <[email protected]>
Signed-off-by: Elena Khaustova <[email protected]>
Signed-off-by: Elena Khaustova <[email protected]>
Signed-off-by: Elena Khaustova <[email protected]>
Signed-off-by: Elena Khaustova <[email protected]>
Signed-off-by: Elena Khaustova <[email protected]>
Signed-off-by: Elena Khaustova <[email protected]>
@ElenaKhaustova ElenaKhaustova enabled auto-merge (squash) July 5, 2024 10:10
@ElenaKhaustova ElenaKhaustova merged commit e04b1ce into main Jul 5, 2024
10 checks passed
@ElenaKhaustova ElenaKhaustova deleted the feature/726-remove-consent-confirmation branch July 5, 2024 10:17
merelcht pushed a commit to galenseilis/kedro-plugins that referenced this pull request Aug 27, 2024
…try` (kedro-org#744)

* Removed consent confirmation

Signed-off-by: Elena Khaustova <[email protected]>

* Updated tests

Signed-off-by: Elena Khaustova <[email protected]>

* Fixed ruff

Signed-off-by: Elena Khaustova <[email protected]>

* Fixed docstring

Signed-off-by: Elena Khaustova <[email protected]>

* Removed debug output

Signed-off-by: Elena Khaustova <[email protected]>

* Dummy commit

Signed-off-by: Elena Khaustova <[email protected]>

* Dummy commit

Signed-off-by: Elena Khaustova <[email protected]>

* Updated huggingface-hub version

Signed-off-by: Elena Khaustova <[email protected]>

* Updated huggingface-hub version

Signed-off-by: Elena Khaustova <[email protected]>

* CI fix

Signed-off-by: Elena Khaustova <[email protected]>

---------

Signed-off-by: Elena Khaustova <[email protected]>
Signed-off-by: Merel Theisen <[email protected]>
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.

4 participants