search

amplitude / Amplitude-TypeScript

TypeScript Amplitude Analytics SDK
https://amplitude.github.io/Amplitude-TypeScript/
MIT License
135 stars 39 forks source link

fix: add metrics field to remote config fetcher #847

Closed Mercy811 closed 3 months ago

Mercy811 commented 3 months ago

Summary

Checklist

lewgordon-amplitude commented 3 months ago

The change makes sense, but I'm wondering the bigger goal here?

Mercy811 commented 3 months ago

Hi @lewgordon-amplitude, the goal is to be able to add more metrics without changing the RemoteConfigFetch interface again and grouping all metrics under the metrics field.

Mercy811 commented 3 months ago

Hi @lewgordon-amplitude,

how these metrics are intended to be used/persisted.

They will be tracked by datadog. Here is the PR to consume these metrics https://github.com/amplitude/nova/pull/18667/files

for multiple remote config calls (which can happen) this will always overwrite the value to the last seen value. It'd be great to track more robust metrics by using multiple events

Remote config fetch call will only happen on SDK initialization. The metrics collected will be sent in the next request payload like https://github.com/amplitude/Amplitude-TypeScript/pull/772. The case where the value is overwritten will be rare. The only case I can imagine is that the user opens a page but didn't track any event so that no request is sent.

lewgordon-amplitude commented 3 months ago

I guess my concern here is that we're making decisions for the behavior of this package based on what we expect the caller will do. I can't imagine this is generally used outside of a few use cases. I also don't want to be a hard blocker here, but I think at the very least we need to document the behavior of these metrics since it'll act different than say a DataDog or Grafana metric API.

Mercy811 commented 3 months ago

Thanks @lewgordon-amplitude, https://github.com/amplitude/Amplitude-TypeScript/pull/847/commits/52bc66c5c604cc024d9d2385e582c4c52b5e2452 takes metrics as enum and I added a few lines of comments there to explain each metric.

Mercy811 commented 3 months ago

Hi @lewgordon-amplitude, I am reviewing the PR again and I don't think we should keep using Map here as the original motivation for using Map was to not change the interface any more when adding a new metric. But now we keep every metrics as enum type, there is no value to keep using Map any more.

I'm thinking about

export interface RemoteConfigFetch<T> extends BaseRemoteConfigFetch<T> {
  // Each metric is independent. See RemoteConfigMetric for more information.
  metrics: RemoteConfigMetric;
}

export interface RemoteConfigMetric {
  // The time, in milliseconds, taken to fetch the last remote config via getRemoteConfig() from IndexedDB.
  // If multiple remote config fetches occur, this value will be updated to reflect the time of the most recent fetch.
  fetchTimeIDB?: number;
  // The time, in milliseconds, taken to fetch the last remote config via getRemoteConfig() from API.
  // If multiple remote config fetches occur, this value will be updated to reflect the time of the most recent fetch.
  fetchTimeAPI?: number;
}

Wdyt?