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

Add telemetry chapter to the docs #3603

Merged
merged 3 commits into from
Feb 14, 2024

Conversation

DimedS
Copy link
Contributor

@DimedS DimedS commented Feb 7, 2024

Description

This PR adds a summary of telemetry information to the Kedro documentation, into the plugins section. It includes a link to the README.md file in the kedro-telemetry repository for detailed information.

Developer Certificate of Origin

We need all contributions to comply with the Developer Certificate of Origin (DCO). All commits must be signed off by including a Signed-off-by line in the commit message. See our wiki for guidance.

If your PR is blocked due to unsigned commits, then you must follow the instructions under "Rebase the branch" on the GitHub Checks page for your PR. This will retroactively add the sign-off to all unsigned commits and allow the DCO check to pass.

Checklist

  • Read the contributing guidelines
  • Signed off each commit with a Developer Certificate of Origin (DCO)
  • 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 RELEASE.md file
  • Added tests to cover my changes
  • Checked if this change will affect Kedro-Viz, and if so, communicated that with the Viz team

@DimedS DimedS marked this pull request as ready for review February 7, 2024 16:23
Copy link
Contributor

@stichbury stichbury left a comment

Choose a reason for hiding this comment

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

I'm guessing we have to use this as legal boilerplate. I am reviewing it as such so not requesting US spelling converted to English here. I think you should probably call it kedro-telemetry rather than Kedro-Telemetry.

Approving because I suspect there's limited wriggle room for changes.

Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

I left a comment about one bit in the text I don't understand which was present in the original one.

Aside from that, I like the text but not where it's placed, inside the "how to extend Kedro" section. I think it's difficult to find, and unrelated to the rest of the content.

Should we add a top-level section that is titled "Privacy policy" or "Telemetry collection" instead, so it's more visible? Maybe under "Learn about Kedro". Similar to other projects kedro-org/kedro-plugins#510 (comment) (see "Documentation" column).

wdyt @stichbury ?

docs/source/extend_kedro/plugins.md Outdated Show resolved Hide resolved
@stichbury
Copy link
Contributor

Should we add a top-level section that is titled "Privacy policy" or "Telemetry collection" instead, so it's more visible? Maybe under "Learn about Kedro". Similar to other projects kedro-org/kedro-plugins#510 (comment) (see "Documentation" column).

wdyt @stichbury ?

I think the placement is OK TBH, since it's covering the telemetry plugin details and the plugin is, in itself, an example of a plugin. I also think moving it to a distinct section is adding the expense of accidental complexity to most readers (who don't care about it) at the benefit of the few who do. I'd personally leave it and signpost it (if we don't already) from where those concerned readers are most likely to be looking (as you suggest, the privacy policy).

Just my 2p though.

@astrojuanlu
Copy link
Member

I'm still not convinced to be honest. That page is about how to extend Kedro through plugins, and contains lots of technical documentation that I'd expect to be there. Adding kedro-telemetry there feels weird - for consistency, we should be adding there a description of all our plugins, which I think it wouldn't make sense and also would obscure the location of the data collection information even more.

I'll come up with a proposal and push directly to this branch.

DimedS and others added 3 commits February 14, 2024 17:58
Signed-off-by: Dmitry Sorokin <dmd40in@gmail.com>
Co-authored-by: Jo Stichbury <jo_stichbury@mckinsey.com>
Signed-off-by: Dmitry Sorokin <40151847+DimedS@users.noreply.github.com>
Signed-off-by: Juan Luis Cano Rodríguez <juan_luis_cano@mckinsey.com>
@astrojuanlu astrojuanlu force-pushed the 508-add-telemetry-chapter-to-kedro-docs branch from ac75d01 to 948e19b Compare February 14, 2024 16:58
@astrojuanlu astrojuanlu enabled auto-merge (squash) February 14, 2024 16:58
@astrojuanlu astrojuanlu merged commit cd026f5 into main Feb 14, 2024
9 checks passed
@astrojuanlu astrojuanlu deleted the 508-add-telemetry-chapter-to-kedro-docs branch February 14, 2024 17:02
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.

Update our kedro-telemetry documentation on which data is collected
3 participants