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.

Sign up for GitHub

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

Jump to bottom

Aligned kedro-viz telemetry with kedro-telemetry #2112

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Aligned kedro-viz telemetry with kedro-telemetry #2112

wants to merge 7 commits into from
+40 −22

Conversation

jitu5
Copy link
Contributor

@jitu5 jitu5 commented Sep 25, 2024 •
edited
Loading

Description

Resolves #2020

This PR updates existing telemetry setup in Kedro-Viz:

  • Added kedro-telemetry as required dependency.
  • Used _check_for_telemetry_consent from kedro-telemetry
    • To check environment variables DO_NOT_TRACK or KEDRO_DISABLE_TELEMETRY
    • To check for consent in .telemetry file
  • Updated get_heap_identity to generate an unique UUID for each user of kedro-telemetry.
  • If kedro-telemetry is not installed, it should be treated as consent being False.

Heap event data: (Identity)
Before:
Screenshot 2024-09-25 at 10 53 46 a m

After:
Screenshot 2024-09-25 at 11 17 17 a m

QA notes

  • All test should pass
  • Screenshot 2024-09-25 at 7 30 10 p m

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added new entries to the RELEASE.md file
  • Added tests to cover my changes

@jitu5
Aligned kedro-viz telemetry with kedro-telemetry
180fc91 
Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>
@jitu5
Test fix
e9d85f4 
Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>
@jitu5
lint fix
d6aa414 
Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>
@jitu5
Test fix
d9f583d 
Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>
@jitu5
Release note added
a458828 
Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>
@jitu5 jitu5 requested a review from DimedS September 25, 2024 18:31
@jitu5 jitu5 self-assigned this Sep 25, 2024
@jitu5 jitu5 marked this pull request as ready for review September 25, 2024 18:33
DimedS
DimedS approved these changes Sep 26, 2024
View reviewed changes
Copy link

@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 @jitu5, LGTM!

ravi-kumar-pilla
ravi-kumar-pilla approved these changes Sep 26, 2024
View reviewed changes
Copy link
Contributor

@ravi-kumar-pilla ravi-kumar-pilla left a comment

Choose a reason for hiding this comment

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

Code looks good to me 💯 ... thank you @jitu5

rashidakanchwala
rashidakanchwala approved these changes Sep 27, 2024
View reviewed changes
Copy link
Contributor

@rashidakanchwala rashidakanchwala 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 @jitu5 , Will have to connect with @merelcht to see if it makes sense to convert the functions to public since Kedro-viz accessed it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DimedS DimedS DimedS approved these changes

@rashidakanchwala rashidakanchwala rashidakanchwala approved these changes

@ravi-kumar-pilla ravi-kumar-pilla ravi-kumar-pilla approved these changes

@astrojuanlu astrojuanlu Awaiting requested review from astrojuanlu astrojuanlu is a code owner

Assignees

@jitu5 jitu5

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update kedro-viz telemetry to reflect recent changes in kedro-telemetry
4 participants
@jitu5 @rashidakanchwala @DimedS @ravi-kumar-pilla