feat(telemetry): Implement telemetry message notification

[DimedS]

Description

This PR implements a new notification that is sent only once per Kedro command execution according to issue #727.

Changes Made

• Removed old notifications and changed the notification type from debug to info.
• Fixed the logger from logging.getLogger(__name__) to logging.getLogger("kedro.telemetry") because the current logger did not work, and notifications from the telemetry module were not visible.
• Implemented the new notification to be sent only once inside the before_command_run() hook, which will be executed before any Kedro command.
• Left the message unchanged for cases when consent is not provided but changed its type from debug to info.
• Added tests to check that the messages are logged correctly.

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

[ElenaKhaustova]

Thank you, @DimedS, LGTM!

I have one question regarding catching general exception inside before_command_run but not in after_catalog_created. Do we need it since we're catching RequestException inside _send_heap_event? Maybe we can replace it to more specific exceptions instead?

[noklam]

Fixed the logger from logging.getLogger(name) to logging.getLogger("kedro.telemetry") because the current logger did not work, and notifications from the telemetry module were not visible.

I wonder is there a better solution for this. This feels hacky as it only works because kedro's logging.yml expose the kedro logger and thus kedro.telemetry would work. Can we set the logging level at a pacakge level instead?

For example: https://github.com/Galileo-Galilei/kedro-mlflow/blob/64b8e94e1dafa02d979e7753dab9b9dfd4d7341c/kedro_mlflow/__init__.py#L7C1-L7C51

[noklam]

What about runs that does not go through CLI, i.e. session.run?

[DimedS]

Thank you, @noklam, good point. As I understand it, in the current PR, telemetry will be sent in the after_catalog_created() hook without user notification. I propose addressing this issue here: GitHub Issue #730.

In that PR, we can modify the logic to send one heap event per user command and update user notifications accordingly:

• If it's a CLI command and the catalog is created, send one heap event in the after_catalog_created() hook along with a user notification message.
• If it's a CLI command and the catalog is not created, send one heap event in the before_command_run() hook along with a user notification message.
• If it's not a CLI command and the catalog is created, send one heap event in the after_catalog_created() hook along with a user notification message.

[noklam]

Agree, let's address this in #730

[DimedS]

Fixed the logger from logging.getLogger(name) to logging.getLogger("kedro.telemetry") because the current logger did not work, and notifications from the telemetry module were not visible.

I wonder is there a better solution for this. This feels hacky as it only works because kedro's logging.yml expose the kedro logger and thus kedro.telemetry would work. Can we set the logging level at a pacakge level instead?

For example: https://github.com/Galileo-Galilei/kedro-mlflow/blob/64b8e94e1dafa02d979e7753dab9b9dfd4d7341c/kedro_mlflow/__init__.py#L7C1-L7C51

Thanks, @noklam. I made that change. Originally I intended to inherit the logging configuration from Kedro settings. Do you think that's not a good approach?

[DimedS]

Thank you, @DimedS, LGTM!

I have one question regarding catching general exception inside before_command_run but not in after_catalog_created. Do we need it since we're catching RequestException inside _send_heap_event? Maybe we can replace it to more specific exceptions instead?

Thank you for the comment, @ElenaKhaustova. I agree that the current try-except blocks might be excessive. I propose addressing this in the final step of the telemetry code refactoring, in issue #730.

[noklam]

Thanks, @noklam. I made that change. Originally I intended to inherit the logging configuration from Kedro settings. Do you think that's not a good approach?

As logging.yml is nothing but Python's logging, I prefer stick to the convention as the logger usually name after the modue/file. This isn't too important as we migrate telemetry back to kedro, this will become an actual kedro module and it should work automatically with the current kedro logger.

[astrojuanlu]

sphinx.errors.SphinxWarning: Summarised items should not include the current module. Replace 'kedro_datasets.api.APIDataset' with 'api.APIDataset'.

[astrojuanlu]

LGTM!