Inject tracking data by jrhone · Pull Request #2110 · streamlit/streamlit

jrhone · 2020-10-07T15:36:26Z

What

We need to inject s4a data into user apps so it gets sent along as tracking data to Segment.
Users should be prevented from injecting this data and messing up our metrics.
Data needs to be injected early in the app lifecycle, before any tracking data is sent.

How

Set a global variable on the window object from s4a, and then the core application will be able to do something like window.parent.trackingData

Data to inject:

hosted: s4a/s4t/localhost
user id: users database id in s4a, hashed for security reasons
github owner
github repo
github branch
github path

Note

I snuck in a change to add the installation id to the event tracking

akrolsmir

Some parts that I was personally confused about:

We need to inject s4a data into user apps so it gets sent along as tracking data to Segment.

Curious, why can't the s4a outer wrapper send this data directly to segment?

hosted: s4a/s4t/localhost

How can "localhost" be chosen if streamlitTracking is only exposed on S4A/S4T?

users database id in s4a, hashed for security reasons

Note that hashing isn't an infallible way of obscuring data (but I'm expecting that this is just one of many security measures.) What happens if the user database id is exposed?

akrolsmir · 2020-10-07T20:05:52Z

frontend/src/lib/MetricsManager.ts

+  // Use the tracking data injected by S4A if the app is hosted there
+  private getHostTrackingData(): Record<string, unknown> {
+    if (
+      window.location.origin.toLowerCase().endsWith(".streamlit.io") &&


Is there a more specific prefix for S4A that we want to use? (share.streamlit.io, s4a.streamlit.io)?

I wonder if we'd want to someday e.g. host apps on docs.streamlit.io but not want to include those in metrics; but we can also just cross that bridge when we get there.

Will double check the specificity of the prefix..

Regarding the docs.streamlit.io example tho.. if it's just a page that has some user apps in iFrames, it would need to set a value for window.streamlitTracking so the embedded user apps tracked it. So by doing nothing, nothing's tracked.

If I didn't get your example right let me know.

jrhone · 2020-10-07T21:57:17Z

Curious, why can't the s4a outer wrapper send this data directly to segment?

The S4A wrapper will have its own separate tracking as well (mainly for things happening in the dashboard), but we also want to tag core's data with some S4A attributes.

jrhone · 2020-10-07T22:04:47Z

How can "localhost" be chosen if streamlitTracking is only exposed on S4A/S4T?

That's a great point. I haven't given too much thought yet to how we'll add the hosted:localhost tag but I think it's out of scope with regards to this PR so will punt for now.

Technically though it shouldn't need streamlitTracking or depend on being in an iFrame.

jrhone · 2020-10-07T22:29:16Z

Note that hashing isn't an infallible way of obscuring data (but I'm expecting that this is just one of many security measures.) What happens if the user database id is exposed?

Well, one case I can think of... if we have any APIs that accept a user_id a malicious user could try to access the api with this information as input, but we should be enforcing proper access controls on our API endpoints.

I suppose if we used auto incrementing IDs then we could be exposing a range of IDs to 3rd parties so they could try to harvest data from multiple users, given the ID of one.

But yea, just adding an additional security measure.

karriebear · 2020-10-08T15:45:30Z

frontend/src/lib/MetricsManager.test.ts

  expect(mm.identify.mock.calls[0][1]).toMatchObject({
    authoremail: SessionInfo.current.authorEmail,
-    machineIdV1: SessionInfo.current.installationIdV1,
-    machineIdV2: SessionInfo.current.installationIdV2,


It looks like machineIdV1 and machineIdV2 should still belong in this object? It doesn't look like there's any conditional statements that would now remove these? Unless you're just covering these with the additional tests down below?

Yea just moved the check for these into the tests below to make it a bit cleaner.

tvst · 2020-10-08T17:24:24Z

frontend/src/App.tsx

 declare global {
  interface Window {
    streamlitDebug: any
+    streamlitTracking: Record<string, unknown>


Can you rename this to something clearer? Like streamlitShareMetadata or something?

Because "Streamlit tracking" could mean all sorts of things...

Renamed to streamlitShareMetadata

tvst · 2020-10-08T19:07:43Z

frontend/src/lib/MetricsManager.ts

+  private static getHostTrackingData(): Record<string, unknown> {
+    if (isInChildFrame() && window.parent.streamlitShareMetadata) {
+      return pick(window.parent.streamlitShareMetadata, [
+        "hosted",


Nit: can we rename this to "hostType" or "hostedAt" ?

Also, this can take values "s4a", "s4t", "localhost", right? But what if the app is in, say, Heroku? Should we have "other" as a valid string too? (Or replace "localhost" with "other" if Heroku shows as localhost today?)

Sure, renaming to hostedAt

Currently we are only assigning S4A as a value to this and there are no restrictions on the list of values we can add.

So we can add other and localhost later on when we think of a way to detect and include this data.

tvst · 2020-10-08T19:08:38Z

frontend/src/lib/MetricsManager.ts

+        "repo",
+        "branch",
+        "mainModule",
+        "creatorId",


Just checking: on the metrics side we also have access to the app's URL, right? Or is that something we need to add to streamlitShareMetadata?

In BigQuery we have the app's URL

# By karrie (7) and others # Via GitHub * develop: Removing cache option from main menu if s4a (streamlit#2149) Fix empty deploy page (streamlit#2148) Don't wait for unit tests before starting Cypress (streamlit#2142) Fix formatting of st.file_uploader docstring (streamlit#2141) Fix broken link in 0.68 changelog (streamlit#2144) Fix useEffect warning (streamlit#2137) Add global GTM container (streamlit#2128) Allow Streamlit server to handle Range Requests (streamlit#1967) rename hosted to hostedAt (streamlit#2132) Update change log Update notices Up version to 0.68.0 Rename hosted to hostedAt in tracking data (streamlit#2132) Inject tracking data (streamlit#2110) [Feature Branch] File uploader (streamlit#2130) links for docs (streamlit#2129) Upgrade ProtobufJS and fix build script (streamlit#2118) Refresh landing page (streamlit#2116) Improve docstrings + tutorials for Layout (streamlit#2117) Better 'streamlit run' error message when no extension provided (streamlit#2115) # Conflicts: # frontend/src/components/elements/Video/Video.tsx # frontend/src/components/widgets/FileUploader/FileUploader.test.tsx # frontend/src/components/widgets/FileUploader/FileUploader.tsx # frontend/src/lib/utils.ts

jrhone added 6 commits October 6, 2020 20:43

cleanup

e3b47be

debugging

4875767

cleanup

8e07e8e

don't use any

b93d594

remove logging

e53c9ca

cleanup

6952f21

akrolsmir reviewed Oct 7, 2020

View reviewed changes

jrhone added 4 commits October 7, 2020 18:37

fix lint

3376584

only check for existence of tracking object

5009a28

cleanup test

84a7d18

add installation ids to events, cleanup tests

f0c3ad6

jrhone marked this pull request as ready for review October 8, 2020 14:55

jrhone requested review from a team October 8, 2020 14:55

karriebear approved these changes Oct 8, 2020

View reviewed changes

tvst reviewed Oct 8, 2020

View reviewed changes

rename streamlitTracking to streamlitShareMetadata

d538d8c

karriebear merged commit 6a1a1d0 into streamlit:develop Oct 8, 2020

tvst reviewed Oct 8, 2020

View reviewed changes

Conversation

jrhone commented Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akrolsmir 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!

Choose a reason for hiding this comment

Uh oh!

jrhone Oct 7, 2020 • 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!

jrhone commented Oct 7, 2020

Uh oh!

jrhone commented Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrhone commented Oct 7, 2020

Uh oh!

karriebear Oct 8, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jrhone commented Oct 7, 2020 •

edited

Loading

akrolsmir left a comment •

edited

Loading

jrhone Oct 7, 2020 •

edited

Loading

jrhone commented Oct 7, 2020 •

edited

Loading

karriebear Oct 8, 2020 •

edited

Loading