feat(telemetry): Combine Telemetry hook to send heap event once

[noklam]

Description

Fix #730

Development notes

The test isn't quite working yet, as some test is passing when run individually but fail when I run in a batch. The change is quite big already so I want to get some review:

• KedroTelemetryCLIHooks and KedroTelemetryProjectHooks are combined to avoid cross hook communication and make it possible to send event ONCE only by storing some intermediate states (handle entrypoint for both kedro command or just session.run()
• Added Integration test as I think it's important to make sure the event is only sent once, mocking wouldn't work so I need a real project here. For the effort I just create a minimal kedro project. I don't think we should spend too much effort as we moved kedro-telemetry back to kedro then we can share the same starter project. It's a temporary structure as I don't think adding it to the current test_plugin.py fits as the test is more high level.
• I created a fake telemetry ID in case something isn't working we would still capture the fake telemetry from ourselves.

[tool.kedro_telemetry]
project_id = "KEDRO_TELEMETRY_TEST"

• As discussed, the almost duplicate CLI command event is removed. kedro-telemetry: Spike to reduce redundant telemetry events  #730 (comment)
  There are more detailed conversation internally for this approach.

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

[DimedS]

Thank you for the PR, @noklam. Great job! I appreciate the approach to merge the two classes into one to facilitate data exchange, and the integration tests are really useful. I mostly agree with the new logic; it's excellent that we can now send only one event! I left a small comment and would like to propose removing the before_command_run() hook entirely. I believe it's not necessary, and all the logic can be placed inside the other two hooks with the same result. Is that correct?

[DimedS]

I believe we don't need that check here. The logic should be that enriched_properties should be sent in any case. It is our responsibility to ensure the event is not sent again in the after_command_run() hook later.

[noklam]

Yeah I think you are right, since we only have two hooks after_catalog_created and after_command_run that will send event, and this is the earliest one so in reality there shouldn't be case this is set True.

[ankatiyar]

Generally, the approach makes sense but I think it would be good to wait to switch to after_command_run till @lrcouto's analysis on the Kedro side is complete (re #709) - i.e. if the hook is executed even if there's an exception during the command execution. Otherwise we miss out on data about commands run with failures.

[DimedS]

Generally, the approach makes sense but I think it would be good to wait to switch to before_command_run till @lrcouto's analysis on the Kedro side is complete (re #709) - i.e. if the hook is executed even if there's an exception during the command execution. Otherwise we miss out on data about commands run with failures.

My opinion is that it would be better to proceed with current approach, but first completely remove the before_command_run() hook for clarity. The logic will be clear: if a step includes catalog creation, enhanced data will be sent in the after_catalog_creation() hook; otherwise, a simple set of telemetry data will be sent at the last step.

I think it's acceptable to lose information about failed runs for now because we are not currently analysing it properly. I believe it makes sense to implement a special flow for failed runs later, such as collecting error types and analysing them for some insights.

[noklam]

@ankatiyar
The after_command_run change is slightly tricky.

There may be conflicts between the two requirements:

1. Send event even if command fails
2. Make sure event is not sent at the beginning and only sent it after the metadata is enriched

[noklam]

I can't approve my own PR, but it looks good to me now :)

[DimedS]

As agreed, we have merged KedroTelemetryCLIHooks and KedroTelemetryProjectHooks into KedroTelemetryHook. I refactored the code to improve clarity and reduce repetition by consolidating relevant parts into the new class. Additionally, I updated the tests to reflect this merge.

The current logic is as follows:

1. We collect information about the CLI command and basic project details in the before_command_run hook.
2. If the after_catalog_created hook is triggered, we add information about nodes and pipelines and then send the event.
3. If step 2 does not occur, we send the event in the after_command_run hook.

I attempted to fix the integration tests but was unsuccessful. After discussing with @noklam, I have moved the integration tests to a separate issue and PR (#771).

Please re-review the PR.

[ankatiyar]

LGTM! I think this partially resolves #709 too then!