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: add telemetry documentation #35

Merged
merged 29 commits into from
Jan 13, 2023
Merged

Conversation

SgtPooki
Copy link
Member

@SgtPooki SgtPooki commented Jan 5, 2023

fixes #29
fixes ipfs/ipfs-gui#125

note that i’ve made some changes to the groupings of metric features:

removed groups: “tracking”, “marketing”
moved items: “events” is now in “minimal” metrics
new groups: “UX” (scrolls, clicks, forms), “Feedback” (star-rating, feedback), “location” (location)
removed items: “attribution”, “users”

cc @BigLep @lidel @tinytb @juliaxbow

Callouts for this PR: This PR is intended to close out #29 & ipfs/ipfs-gui#125

Please bring up any concerns you have that I missed

@SgtPooki SgtPooki requested review from whizzzkid and BigLep January 5, 2023 23:24
@SgtPooki SgtPooki self-assigned this Jan 5, 2023
@SgtPooki SgtPooki marked this pull request as ready for review January 5, 2023 23:26
@SgtPooki SgtPooki requested a review from 0xDanomite January 5, 2023 23:26
@SgtPooki SgtPooki mentioned this pull request Jan 5, 2023
12 tasks
src/MetricsProvider.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@whizzzkid whizzzkid left a comment

Choose a reason for hiding this comment

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

thanks for documenting this, some nits.

docs/telemetry/CollectedData.template.md Outdated Show resolved Hide resolved
docs/telemetry/CollectedData.template.md Outdated Show resolved Hide resolved
docs/telemetry/CollectionPolicy.md Outdated Show resolved Hide resolved
docs/telemetry/CollectionPolicy.md Outdated Show resolved Hide resolved
docs/telemetry/CollectionPolicy.md Outdated Show resolved Hide resolved
docs/telemetry/CollectionPolicy.md Outdated Show resolved Hide resolved
docs/telemetry/CollectionPolicy.md Outdated Show resolved Hide resolved
docs/telemetry/CollectionPolicy.md Outdated Show resolved Hide resolved
docs/telemetry/FAQs.md Show resolved Hide resolved
src/MetricsProvider.ts Outdated Show resolved Hide resolved
Comment on lines 34 to 36
* https://github.com/ipfs-shipyard/ignite-metrics/blob/feature/metric-library/129/docs/telemetry/CollectionPolicy.md
* https://github.com/ipfs-shipyard/ignite-metrics/blob/feature/metric-library/129/docs/telemetry/PrivacyPolicy.md
* https://github.com/ipfs-shipyard/ignite-metrics/blob/feature/metric-library/129/docs/telemetry/FAQs.md
Copy link
Member Author

Choose a reason for hiding this comment

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

This will need to point to main/ prior to merging

Copy link
Member

Choose a reason for hiding this comment

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

(just an idea for the future)
We could replace these with aliases at ipfs.fyi (https://github.com/ipfs/ipfs.fyi):

/telemetry-collection-policy https://github.com...CollectionPolicy.md
/telemetry-privacy-policy    https://github.com...PrivacyPolicy.md
/telemetry-faq               https://github.com...FAQs.md

Copy link
Member Author

Choose a reason for hiding this comment

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

(just an idea for the future) We could replace these with aliases at ipfs.fyi (https://github.com/ipfs/ipfs.fyi):

/telemetry-collection-policy https://github.com...CollectionPolicy.md
/telemetry-privacy-policy    https://github.com...PrivacyPolicy.md
/telemetry-faq               https://github.com...FAQs.md

I love the idea but I'm not sure exactly how this works and there doesn't seem to be enough info in the readme. Are we supposed to create a _redirects file in each repo, or just in this repo?

Co-authored-by: Nishant Arora <1895906+whizzzkid@users.noreply.github.com>
SgtPooki and others added 3 commits January 6, 2023 18:20
Co-authored-by: Nishant Arora <1895906+whizzzkid@users.noreply.github.com>
Co-authored-by: Nishant Arora <1895906+whizzzkid@users.noreply.github.com>
Co-authored-by: Nishant Arora <1895906+whizzzkid@users.noreply.github.com>
@BigLep
Copy link
Contributor

BigLep commented Jan 7, 2023

I got to talk with @SgtPooki and @juliaxbow on 2023-01-06 and relay my mental model of:

  1. reducing complexity for implementation/testing/maintenance
  2. generally maximizing the amount of cases where we're collecting data (so we can be knowledgable about usage) while also not obsessing over it
  3. be able to look at user in the face and feel good about the decisions that are being made

More of my specific comments were captured in that verbal conversation and in https://www.notion.so/pl-strflt/Telemetry-b005d4f217f44db3986902c67d922cf4

I expect to be fully tied up the week of 2023-01-09 and I don't want this work to block on me. I believe I got my perspective across verbally and at this point the only "sign off" I want to make sure we have is from @lidel . (I'd encourage a sync chat the week of 2023-01-09 so can close out on decisions quickly.). I( didn't read this PR - only https://www.notion.so/pl-strflt/Telemetry-b005d4f217f44db3986902c67d922cf4 )

Summary: I'm not a blocking reviewer, but I would like @lidel to be.

Thanks all for navigating here.

Co-authored-by: Nishant Arora <1895906+whizzzkid@users.noreply.github.com>
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

We've discussed this plan during a call today, and the proposed telemetry gathering focused on feature usage metrics is sensible. As noted before, I am okay with opt-out telemetry (like Firefox) as long we have a Privacy Policy and 👍 from Legal.

Fine to merge this PR as documentation to unblock things like ipfs/ipfs-companion#1117

Noting the small caveat I mentioned during the call:

  • We can't ship telemetry enabled by default in webui/desktop without having a privacy policy document first.

README.md Outdated Show resolved Hide resolved
docs/telemetry/CollectionPolicy.md Outdated Show resolved Hide resolved
docs/telemetry/CollectionPolicy.md Outdated Show resolved Hide resolved
docs/telemetry/CollectionPolicy.md Outdated Show resolved Hide resolved
docs/telemetry/CollectionPolicy.md Outdated Show resolved Hide resolved
docs/telemetry/FAQs.md Outdated Show resolved Hide resolved
docs/telemetry/PrivacyPolicy.md Outdated Show resolved Hide resolved
docs/telemetry/CollectedData.template.md Outdated Show resolved Hide resolved
Comment on lines 34 to 36
* https://github.com/ipfs-shipyard/ignite-metrics/blob/feature/metric-library/129/docs/telemetry/CollectionPolicy.md
* https://github.com/ipfs-shipyard/ignite-metrics/blob/feature/metric-library/129/docs/telemetry/PrivacyPolicy.md
* https://github.com/ipfs-shipyard/ignite-metrics/blob/feature/metric-library/129/docs/telemetry/FAQs.md
Copy link
Member

Choose a reason for hiding this comment

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

(just an idea for the future)
We could replace these with aliases at ipfs.fyi (https://github.com/ipfs/ipfs.fyi):

/telemetry-collection-policy https://github.com...CollectionPolicy.md
/telemetry-privacy-policy    https://github.com...PrivacyPolicy.md
/telemetry-faq               https://github.com...FAQs.md

docs/telemetry/CollectionPolicy.md Outdated Show resolved Hide resolved
@SgtPooki
Copy link
Member Author

@lidel

We've discussed this plan during a call today, and the proposed telemetry gathering focused on feature usage metrics is sensible. As noted before, I am okay with opt-out telemetry (like Firefox) as long we have a Privacy Policy and 👍 from Legal.

We got the 👍 from legal to proceed with opt-out metrics for all projects. We will have a privacy policy from legal but do not have one yet (ETA was 1-2 weeks last week)

Fine to merge this PR as documentation to unblock things like ipfs/ipfs-companion#1117

Noting the small caveat I mentioned during the call:

  • We can't ship telemetry enabled by default in webui/desktop without having a privacy policy document first.

I must have misheard during the call. Where is this constraint coming from? Legal explicitly told us gathering app metrics is not blocked by having a privacy policy.

I want to unblock the work required for webui & desktop. How would you recommend I update the content so that the following constraints are met:

  1. Telemetry changes intended for webui & desktop are unblocked
  2. PrivacyPolicy is an obvious placeholder until we get one from Legal

Two potential options:

  1. Referencing the PrivacyPolicy in ipfs-companion (since Legal gave the go-ahead on changes to ipfs-companion without an update to its privacy policy)
  2. Copying the PrivacyPolicy from ipfs-companion, with a note at the top mentioning that it was copied and will be updated soon.

@lidel
Copy link
Member

lidel commented Jan 11, 2023

We can't ship telemetry enabled by default in webui/desktop without having a privacy policy document first.
Where is this constraint coming from? Legal explicitly told us gathering app metrics is not blocked by having a privacy policy.

I was under the impression that we will link to Privacy Policy and Telemetry Collection Policy documents on the Settings screen near the opt-out button, so the user can read them before deciding to opt-out.

If Legal said shipping without a Privacy Policy is fine, and a proper PP document will land eventually, then a placeholder referencing companion policy for now should be good enough 👍 , but please remove reference to the nonsense protocol.ai/legal/#privacy-policy 😆

SgtPooki and others added 9 commits January 12, 2023 14:14
SgtPooki added a commit to ipfs/ipfs-companion that referenced this pull request Jan 12, 2023
@SgtPooki
Copy link
Member Author

SgtPooki commented Jan 13, 2023

I was under the impression that we will link to Privacy Policy and Telemetry Collection Policy documents on the Settings screen near the opt-out button, so the user can read them before deciding to opt-out.

@lidel cc @juliaxbow @whizzzkid

We can add links to both of those if we feel it's necessary, but I would prefer a single location to direct users to, such as the COLLECTION_POLICY in this repo, which links to the PRIVACY_POLICY. Does it make sense to have a separate landing page for users that's more concise? i.e. ./docs/telemetry/README.md ?


ninjaedit:

I threw together a quick demo of what a README.md would allow us to do.. link to https://github.com/ipfs-shipyard/ignite-metrics/tree/feature/telemetry-documentation-landing-page/docs/telemetry

the link would be https://github.com/ipfs-shipyard/ignite-metrics/tree/main/docs/telemetry once merged

@SgtPooki SgtPooki merged commit 4cb35ba into main Jan 13, 2023
@SgtPooki SgtPooki deleted the feature/telemetry-documentation branch January 13, 2023 01:24
github-actions bot pushed a commit that referenced this pull request Jan 13, 2023
## [1.0.2](v1.0.1...v1.0.2) (2023-01-13)

### Documentation

* add telemetry documentation ([#35](#35)) ([4cb35ba](4cb35ba))
@github-actions
Copy link

🎉 This PR is included in version 1.0.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

SgtPooki added a commit to ipfs/ipfs-companion that referenced this pull request Jan 27, 2023
* feat: add types for state

* tmp

* feat: ipfs-companion tracks views and sessions

* Update add-on/src/lib/telemetry.js

Co-authored-by: Nishant Arora <1895906+whizzzkid@users.noreply.github.com>

* Update add-on/src/lib/telemetry.js

Co-authored-by: Nishant Arora <1895906+whizzzkid@users.noreply.github.com>

* Update add-on/src/lib/telemetry.js

Co-authored-by: Nishant Arora <1895906+whizzzkid@users.noreply.github.com>

* Update add-on/_locales/en/messages.json

Co-authored-by: Marcin Rataj <lidel@lidel.org>

* Update add-on/_locales/en/messages.json

Co-authored-by: Marcin Rataj <lidel@lidel.org>

* Update add-on/_locales/en/messages.json

Co-authored-by: Marcin Rataj <lidel@lidel.org>

* Update add-on/_locales/en/messages.json

Co-authored-by: Marcin Rataj <lidel@lidel.org>

* chore: fix options and state typings

* chore: use debug logger

* fix(lint): remove unused method

* fix(lint): run 'npm run fix:lint'

* chore: build and lint success

* chore(types): fix type errors

* chore: add docs/telemetry/COLLECTED_DATA.md

see ipfs-shipyard/ignite-metrics#35

* chore: update old metric group names in logConsent

* chore: clean up UI

* chore: use ignite-metrics from npm

* chore: update ignite-metrics and some types

* fix(tests): tests dont fail on countly-sdk-web import

* fix: build

* chore: temporarily use updated ignite-metrics

* chore: use deployed ignite-metrics version

* chore: address PR comments

* use latest ignite-metrics library
* don't use singleton function for grabbing metricsProvider
* ensure metrics initialize and update properly

* chore: pin ignite-metrics dependency

* chore(lint): fix lint errors

* fix: use browser.runtime.sendMessage

* Telemetry messages are passed between contexts using browser.runtime
* Upgraded to @ipfs-shipyard/ignite-metrics@1.3.0
* Updated consent handling from state to be more explicit

---------

Co-authored-by: Nishant Arora <1895906+whizzzkid@users.noreply.github.com>
Co-authored-by: Marcin Rataj <lidel@lidel.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docs: update metrics documentation Define basic metrics and patterns for consent & collection.
5 participants