[Refactor] Metrics Provider Transition #9729

mayagbarnes · 2024-10-24T19:44:22Z

Describe your changes

Final feature PR to execute the (soft) transition our metrics provider. We plan to use both systems for one release, allowing us to test/compare/adjust based on metrics at scale through the 1.40 release. We will then remove legacy provider code & switch to the production webhook after validating the new system is working as expected.

This PR consolidates the following logic from PRs into this branch:

Outside these PRs:

one commit was added to ensure consistency of anonymousIds between the two systems by manually setting the anonymousId for Segment
another commit tweaked how anonymousId was set in the cookie from localStorage to address an issue in Community Cloud where "%22" was showing up as part of the anonymousId
Enforced stricter typing in MetricsManager with new MetricsEvent proto

Testing Plan

Unit Tests: ✅
Manual Testing: ✅

frontend/app/src/MetricsManager.ts

Initial renaming of `SegmentMetricsManager` class, files, & imports to `MetricsManager` - setting stage for telemetry migration

**Metrics Migration Part 1**: This PR extends the `host_config` endpoint to add a `metricsUrl`, which will serve as the destination URL for metrics data. `metricsUrl` for `host_config` endpoint will handle 4 options: * **`"off"`**: Turns off metrics collection * **`"<url>"`**: Sends metrics events to the passed URL * _The logic to build/send event with necessary fields will be added in a separate PR_ * **`"postMessage"`**: Sends metrics events via `postMessage` API * _This logic will be added in a separate PR_ * **`" "`**: Default (OS) - telemetry destination url will be fetched Should no url be received from the BE, the FE will attempt to fetch the telemetry url from `data.streamlit.io/metrics.json`, caching it in localStorage if successful. If there is no `metricsUrl`, no telemetry is sent.

Update the `GUEST_READY` message payload to provide more information on Streamlit load time

**Metrics Migration Part 2:** This PR adds a `MetricsEvent` proto for a single source of truth for event fields, as well as the infrastructure necessary to send event data to the testing Fivetran webhook in parallel with Segment. This includes building the `anonymousId` & `context` data fields for FW that we received by default from Segment.

**Metrics Migration Part 3:** As a follow up to the metrics options in hostConfig endpoint (PR #9611), this PR adds the ability to send metrics events to the host via postMessage API using our existing Host <-> Guest communication.

frontend/lib/src/hostComm/types.ts

frontend/app/src/MetricsManager.ts

proto/streamlit/proto/MetricsEvent.proto

frontend/app/src/MetricsManager.ts

lukasmasuch

I think the requestDefaultMetricsConfig might need to be awaited. Otherwise, LGTM 👍

lukasmasuch · 2024-10-26T04:36:11Z

frontend/app/src/index.tsx

 */

+// Timestamp when the Streamlit execution started for GUEST_READY message
+const streamlitExecutionStartedAt = Date.now()


I'm a bit on the fence if we should pass this through our app stack as a prop since its a global variable that does not change after initialization. The two alternatives are: 1) Put this variable into the context for this prop, see WindowDimensions as an example
2) Put it into a global window variable, e.g. window.streamlitExecutionStartedAt and read it out when the GUEST_READY message is send.

But this could easily be discussed and implemented as a follow-up after cut off. The way it's implemented right now is fine.

FWIW I also preferred window variable to passing as prop, though Pat & Benny leaned toward prop (discussion here). Now at least votes are tied 😂

lukasmasuch · 2024-10-29T20:12:18Z

frontend/app/src/MetricsManager.ts

+  public initialize({
+    gatherUsageStats,
+    sendMessageToHost,
+  }: {
+    gatherUsageStats: boolean
+    sendMessageToHost: (message: IGuestToHostMessage) => void
+  }): void {
+    this.initialized = true
+    this.sendMessageToHost = sendMessageToHost
+    // Handle if the user or the host has disabled metrics
+    this.actuallySendMetrics = gatherUsageStats && this.metricsUrl !== "off"
+    this.getAnonymousId()
+
+    // Trigger fallback to fetch default metrics config if not provided by host
+    if (this.actuallySendMetrics && !this.metricsUrl) {
+      this.requestDefaultMetricsConfig()
+
+      // If metricsUrl still undefined, deactivate metrics
+      if (!this.metricsUrl) {
+        logError("Undefined metrics config - deactivating metrics tracking.")
+        this.actuallySendMetrics = false
+      }
+    }
+
+    if (this.actuallySendMetrics) {
+      // Segment will not initialize if this is rendered with SSR
+      initializeSegment()
+      this.sendPendingEvents()
+    }
+
+    logAlways("Gather usage stats: ", this.actuallySendMetrics)
+  }


I think the initialize needs to be async and await the requestDefaultMetricsConfig method. Otherwise, the metrics gathering might be deactivated because the async function has not yet finished execution. And it would probably be good to move the this.initialized = true to the end, so that it can initialize async and collect pending events at the same time.

public async initialize({ gatherUsageStats, sendMessageToHost, }: { gatherUsageStats: boolean sendMessageToHost: (message: IGuestToHostMessage) => void }): Promise<void> { this.sendMessageToHost = sendMessageToHost // Handle if the user or the host has disabled metrics this.actuallySendMetrics = gatherUsageStats && this.metricsUrl !== "off" this.getAnonymousId() // Trigger fallback to fetch default metrics config if not provided by host if (this.actuallySendMetrics && !this.metricsUrl) { await this.requestDefaultMetricsConfig() // If metricsUrl still undefined, deactivate metrics if (!this.metricsUrl) { logError("Undefined metrics config - deactivating metrics tracking.") this.actuallySendMetrics = false } } if (this.actuallySendMetrics) { // Segment will not initialize if this is rendered with SSR initializeSegment() this.sendPendingEvents() } logAlways("Gather usage stats: ", this.actuallySendMetrics) this.initialized = true }

Final feature PR to execute the (soft) transition of our metrics provider

mayagbarnes added impact:internal PR changes only affect internal code change:refactor PR contains code refactoring without behavior change labels Oct 24, 2024

mayagbarnes force-pushed the metrics-migration branch from cf785db to 7ed2180 Compare October 24, 2024 19:58

github-advanced-security bot found potential problems Oct 24, 2024

View reviewed changes

frontend/app/src/MetricsManager.ts Fixed Show fixed Hide fixed

frontend/app/src/MetricsManager.ts Fixed Show fixed Hide fixed

mayagbarnes force-pushed the metrics-migration branch from 7ed2180 to 1780fac Compare October 24, 2024 21:11

mayagbarnes and others added 7 commits October 24, 2024 14:38

Rename SegmentMetricsManager (#9610)

d4f9ffe

Initial renaming of `SegmentMetricsManager` class, files, & imports to `MetricsManager` - setting stage for telemetry migration

Update GUEST_READY message (#9702)

7a1067b

Update the `GUEST_READY` message payload to provide more information on Streamlit load time

Add option to postMessage metrics (#9704)

ee4bd97

**Metrics Migration Part 3:** As a follow up to the metrics options in hostConfig endpoint (PR #9611), this PR adds the ability to send metrics events to the host via postMessage API using our existing Host <-> Guest communication.

Fix lint

766489c

Override Segment anonymousId system

e2e8415

mayagbarnes force-pushed the metrics-migration branch from 1780fac to e2e8415 Compare October 24, 2024 23:04

mayagbarnes marked this pull request as ready for review October 24, 2024 23:57

mayagbarnes requested a review from a team as a code owner October 24, 2024 23:57

mayagbarnes force-pushed the metrics-migration branch from a1b7b97 to 3589805 Compare October 25, 2024 01:56

Test cookie fix

306af1d

mayagbarnes force-pushed the metrics-migration branch from 3589805 to 306af1d Compare October 25, 2024 03:28

mayagbarnes added 2 commits October 24, 2024 23:55

Remove test logging

2eb7b23

Merge branch 'develop' into metrics-migration

d38137b

mayagbarnes requested a review from lukasmasuch October 25, 2024 19:33

lukasmasuch reviewed Oct 26, 2024

View reviewed changes

frontend/app/src/MetricsManager.ts Outdated Show resolved Hide resolved

mayagbarnes added 4 commits October 28, 2024 11:19

Address PR comments

2f0613b

Merge branch 'develop' into metrics-migration

a1642d0

Merge branch 'develop' into metrics-migration

ce24b3e

Update to window.localStorage

647fbcf

mayagbarnes added the security-assessment-completed Security assessment has been completed for PR label Oct 29, 2024

lukasmasuch approved these changes Oct 29, 2024

View reviewed changes

Revise initialize func

acd0aa8

sfc-gh-kmcgrady approved these changes Oct 30, 2024

View reviewed changes

mayagbarnes merged commit 245d749 into develop Oct 30, 2024

mayagbarnes deleted the metrics-migration branch October 30, 2024 08:30

edegp pushed a commit to edegp/streamlit that referenced this pull request Jan 19, 2025

[Refactor] Metrics Provider Transition (streamlit#9729)

a23e7dd

Final feature PR to execute the (soft) transition of our metrics provider

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Refactor] Metrics Provider Transition #9729

[Refactor] Metrics Provider Transition #9729

Uh oh!

mayagbarnes commented Oct 24, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lukasmasuch left a comment

Uh oh!

lukasmasuch Oct 26, 2024 •

edited

Loading

Uh oh!

mayagbarnes Oct 29, 2024

Uh oh!

lukasmasuch Oct 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[Refactor] Metrics Provider Transition #9729

[Refactor] Metrics Provider Transition #9729

Uh oh!

Conversation

mayagbarnes commented Oct 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe your changes

Testing Plan

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lukasmasuch left a comment

Choose a reason for hiding this comment

Uh oh!

lukasmasuch Oct 26, 2024 • 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!

mayagbarnes Oct 29, 2024

Choose a reason for hiding this comment

Uh oh!

lukasmasuch Oct 29, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mayagbarnes commented Oct 24, 2024 •

edited

Loading

lukasmasuch Oct 26, 2024 •

edited

Loading