feat: add telemetry to companion by SgtPooki · Pull Request #1117 · ipfs/ipfs-companion

SgtPooki · 2022-12-16T20:35:01Z

Things implemented:

View tracking of companion pages
Session tracking (powering down the extension ends your session and stops tracking sessions)
consent management: disabling telemetry (opting-out) prevents the above two things from happening, but we do send an event indicating that you've opted-out

Things to do

Add metrics tracking notice to IPFS Companion #1114
Fix CI failures
Need to discuss with @BigLep and @TinyTb w.r.t. their discussion on 2023-01-05 whether Add metrics tracking notice to IPFS Companion #1114 needs to be a pre-req for merging this

SgtPooki · 2022-12-17T03:05:10Z

@juliaxbow I would love your opinion on the formatting of the telemetry section

juliaxbow · 2022-12-17T04:12:58Z

Descriptions

Telemetry section In the description, would say "To assist in our efforts, please consent to the collection of these metrics by toggling "on" in the corresponding section. You can change this setting at any time by navigating back to the Companion Preferences page." It's a little unusual to offer consent via a toggle but I think it's ok?
Is the toggle default to on? If so, we should change the wording even further. Something like "IPFS Companion is collecting minimal telemetry data to improve and prioritize our work. You may opt out of the collection of these metrics by toggling "off" in the corresponding section below. You can change this setting ......"
Metrics Section the metric collection description, I would change wording to "Sends ‘session’ and ‘view’ events only when ipfs-companion is enabled and your browser is not idle." Current wording raises the question of who is sending this information where?
The section header "minimal metrics: ..." makes it seem like there are additional, more intrusive metrics somewhere. Until we have other metrics we're tracking I think we can just call it "Session & Companion views." The Telemetry description already points to the fact that these are minimal.

Other

"Read More" links on the page have a link-out icon, so would add that in. (in the screenshot below i have the same link blue because I think all link-out icons should look like links but keep consistent with the rest of the page for now ;) )
The toggle is centered but it doesn't look like it is. Until we have more sections & metrics that we're tracking, I would move it up to be top-aligned with the first sentence in the description. I know this is inconsistent with all other toggles on the page but to me it's more clear what the user is agreeing to. This is subjective, though, so feel free to ignore.

Incl screenshot to show the suggested toggle alignment

whizzzkid

LGTM in principle, a few nits, I need to validate this locally though.

add-on/src/lib/telemetry.js

whizzzkid · 2022-12-17T08:22:37Z

add-on/src/lib/telemetry.js

+}
+
+/**
+ * @param {ReturnType<import('./state').initState>['options']} state


can we import types form the declaration from the lib?

add-on/src/lib/telemetry.js

whizzzkid · 2022-12-17T08:27:27Z

add-on/src/lib/telemetry.js

@@ -0,0 +1,60 @@
+
+import { MetricsProvider } from '@ipfs-shipyard/ignite-metrics/vanilla'


@ipfs-shipyard/ignite-metrics/vanilla

This needs to be mapped correctly, this breaks the build.

This should be working properly with latest release of ignite-metrics

lidel

Looks sensible, as long we address below:

We should not be sending requests to Countly every minute a user uses a browser.
(I assume that is what is happening? I was unable to build this and test locally)
- This causes many issues, for example drains laptop battery by waking up WIFI when no internet conneciton would be needed otherwise.
- As discussed, figure a way to batch things so we report them only once an hour or once a day.
Regular people don't know what 'session' and 'view' means.
- See my suggestions below on humanizing the language :)

add-on/_locales/en/messages.json

add-on/src/lib/options.js

add-on/src/lib/telemetry.js

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

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

see ipfs-shipyard/ignite-metrics#35

…anion

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

SgtPooki · 2023-01-27T18:09:57Z

FYI @lidel @whizzzkid I had some changes locally that weren't pushed up... getting those up now

* 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

…anion

whizzzkid

LGTM, thanks!

whizzzkid · 2023-01-27T20:35:55Z

add-on/src/lib/ipfs-companion.js

+    }
+  }
+
+  function onTelemetryMessage (request, sender) {


whizzzkid · 2023-01-27T20:38:30Z

add-on/src/lib/ipfs-companion.js

          await registerSubdomainProxy(getState, runtime)
          shouldRestartIpfsClient = true
          shouldStopIpfsClient = !state.active
+          state.active ? startSession() : endSession()


for now this should be ok, in the future we might wanna have a message being broadcast and such listeners can subscribe to those.

whizzzkid · 2023-01-27T20:38:50Z

add-on/src/options/page.js

+  ${telemetryForm({
+    telemetryGroupMinimal: state.options.telemetryGroupMinimal,
+    telemetryGroupMarketing: state.options.telemetryGroupMarketing,
+    telemetryGroupPerformance: state.options.telemetryGroupPerformance,
+    telemetryGroupTracking: state.options.telemetryGroupTracking,
+    onOptionChange
+  })}


@SgtPooki was this updated?

whizzzkid · 2023-01-27T20:40:03Z

tsconfig.json

+    "emitDeclarationOnly": true,
+    "declaration": true,
+    "moduleResolution": "nodenext",
+    "noImplicitAny": false


Thanks for adding tsconfig.json and we can fix this when we have a better handle of things.

whizzzkid · 2023-01-27T21:00:28Z

@lidel I think the default flush interval is good enough for now, we can revisit this later if needed.

figure a way to batch things so we report them only once an hour or once a day.

for now I think we can merge this and release as is.

SgtPooki · 2023-01-27T21:02:14Z

Agreed with @whizzzkid. Metrics are only sent (currently) when interacting with the extension (enabling/disabling, viewing different pages), so battery use should not be a concern.

* main: feat: add telemetry to companion (#1117) chore: update PRIVACY-POLICY.md (#1137)

SgtPooki added 2 commits December 15, 2022 18:13

feat: add types for state

dbde5bd

tmp

3e3dc16

SgtPooki linked an issue Dec 16, 2022 that may be closed by this pull request

Feat: getting basic metrics in ipfs-companion #1115

Closed

4 tasks

feat: ipfs-companion tracks views and sessions

829283e

SgtPooki requested a review from whizzzkid December 17, 2022 03:05

whizzzkid approved these changes Dec 17, 2022

View reviewed changes

whizzzkid reviewed Dec 17, 2022

View reviewed changes

SgtPooki self-assigned this Jan 3, 2023

whizzzkid mentioned this pull request Jan 6, 2023

feat: track intercepted URL count (app usage) #1127

Open

3 tasks

lidel requested changes Jan 11, 2023

View reviewed changes

lidel mentioned this pull request Jan 11, 2023

feat: add telemetry documentation ipfs-shipyard/ignite-metrics#35

Merged

SgtPooki and others added 17 commits January 12, 2023 12:33

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

2ab0bbe

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

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

1934425

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

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

faf72a8

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

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

3b83c17

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

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

f6b6081

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

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

e372e74

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

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

11d3b9b

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

chore: fix options and state typings

4a0af8f

chore: use debug logger

a256e05

fix(lint): remove unused method

798c5fe

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

8d4d6c8

chore: build and lint success

4058da2

chore(types): fix type errors

0e66ea9

chore: add docs/telemetry/COLLECTED_DATA.md

090d700

see ipfs-shipyard/ignite-metrics#35

chore: update old metric group names in logConsent

d83de1a

chore: clean up UI

ceb65b6

chore: use ignite-metrics from npm

9db9b6e

SgtPooki mentioned this pull request Jan 18, 2023

chore: fix ipfs-companion import error ipfs-shipyard/ignite-metrics#60

Merged

SgtPooki changed the title ~~[WIP] feat: add telemetry to companion~~ feat: add telemetry to companion Jan 18, 2023

SgtPooki marked this pull request as ready for review January 18, 2023 17:40

SgtPooki requested review from a team and meandavejustice as code owners January 18, 2023 17:40

SgtPooki added 2 commits January 18, 2023 09:40

Merge branch 'main' into 1115-feat-getting-basic-metrics-in-ipfs-comp…

d334ec0

…anion

Merge branch 'main' into 1115-feat-getting-basic-metrics-in-ipfs-comp…

162aaa8

…anion

SgtPooki requested a review from lidel January 27, 2023 00:57

SgtPooki mentioned this pull request Jan 27, 2023

Feature: Ensure we have opt-out metrics for all of our projects ipfs/ipfs-gui#105

Closed

SgtPooki added 3 commits January 27, 2023 10:08

chore: temporarily use updated ignite-metrics

089b0d0

chore: use deployed ignite-metrics version

ee060c2

chore: address PR comments

ea798df

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

SgtPooki added 4 commits January 27, 2023 10:16

chore: pin ignite-metrics dependency

dad1bfa

chore(lint): fix lint errors

d2f9cab

fix: use browser.runtime.sendMessage

0810499

* 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

Merge branch 'main' into 1115-feat-getting-basic-metrics-in-ipfs-comp…

7160856

…anion

whizzzkid approved these changes Jan 27, 2023

View reviewed changes

SgtPooki merged commit 42eed02 into main Jan 27, 2023

SgtPooki deleted the 1115-feat-getting-basic-metrics-in-ipfs-companion branch January 27, 2023 21:03

whizzzkid mentioned this pull request Jan 27, 2023

Telemetry: Add Companion Version as a Segment #1138

Closed

lidel mentioned this pull request Jan 30, 2023

Telemetry: click count per menu option #1141

Closed

whizzzkid added a commit that referenced this pull request Jan 31, 2023

Merge branch 'main' into feat/recover-client

c7e6956

* main: feat: add telemetry to companion (#1117) chore: update PRIVACY-POLICY.md (#1137)

This was referenced Feb 3, 2023

chore(main): release 2.21.0 #1144

Closed

chore(main): release 2.21.0 #1146

Closed

chore(main): release 2.21.0 #1148

Closed

github-actions bot mentioned this pull request Feb 16, 2023

chore(main): release ipfs-companion 2.23.0 #1159

Closed

github-actions bot mentioned this pull request Sep 21, 2023

chore(main): release 3.0.1 #1288

Merged

		@@ -0,0 +1,60 @@

		import { MetricsProvider } from '@ipfs-shipyard/ignite-metrics/vanilla'

Conversation

SgtPooki commented Dec 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Things implemented:

Things to do

Uh oh!

SgtPooki commented Dec 17, 2022

Uh oh!

juliaxbow commented Dec 17, 2022

Uh oh!

whizzzkid left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

whizzzkid Dec 17, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

whizzzkid Dec 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SgtPooki Jan 13, 2023

Choose a reason for hiding this comment

Uh oh!

lidel left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SgtPooki commented Jan 27, 2023

Uh oh!

whizzzkid left a comment

Choose a reason for hiding this comment

Uh oh!

whizzzkid Jan 27, 2023

Choose a reason for hiding this comment

Uh oh!

whizzzkid Jan 27, 2023

Choose a reason for hiding this comment

Uh oh!

whizzzkid Jan 27, 2023

Choose a reason for hiding this comment

Uh oh!

whizzzkid Jan 27, 2023

Choose a reason for hiding this comment

Uh oh!

whizzzkid commented Jan 27, 2023

Uh oh!

SgtPooki commented Jan 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

SgtPooki commented Dec 16, 2022 •

edited

Loading

whizzzkid Dec 17, 2022 •

edited

Loading

lidel left a comment •

edited

Loading