Create a simple event tracking package #472

camerow · 2024-09-24T20:30:42Z

Overview

Created two entry points for analytics package: one for web using analytics browser client from segment, and one for native using segment client from react native.
Injected into a generic analytics client with configurable options.
Planning to make generic functions private and add specific methods for tracking events.
Proposing to use uppercase or human-readable event names for better readability.
Updating functions to be more generic and customizable via options.

Assumptions

Making generic functions private will not impact current usage in apps.
Uppercase event names will improve readability and consumption.
Adding customizable options for functions will enhance flexibility.

Future steps

I decided to just create the package and not include it anywhere for now. This is just to scope out the work such that we can identify more clearly when we added this to the respective applications, update stakeholders on name changes, etc.

codecov · 2024-09-24T20:43:33Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 22.38%. Comparing base (936fa26) to head (143a8c5).
Report is 14 commits behind head on dev.

Additional details and impacted files

@@           Coverage Diff           @@
##              dev     #472   +/-   ##
=======================================
  Coverage   22.38%   22.38%           
=======================================
  Files         132      132           
  Lines        5521     5521           
  Branches      241      241           
=======================================
  Hits         1236     1236           
  Misses       4285     4285

Components	Coverage Δ
bitcoin	`53.77% <ø> (ø)`
query	`12.05% <ø> (ø)`
utils	`52.45% <ø> (ø)`
crypto	`69.40% <ø> (ø)`
stacks	`53.27% <ø> (ø)`

fbwoolf · 2024-09-24T21:06:18Z

Is there an issue to link here?

camerow · 2024-09-24T21:07:50Z

Is there an issue to link here?

There was but I'm struggling to find it. I think I left it in "draft" and possibly lost it.

camerow · 2024-09-24T21:11:08Z

~~How does the team feel about reusing issues for multiple PRs? I am re-creating this issue, and wondering do we normally create an issue per task or can one issue be represented in multiple PRs?~~

EDIT: Feel like I answered my own question looking at how these things are linked. One issue per PR seems to be the only real logical choice.

fbwoolf · 2024-09-24T23:15:13Z

~~How does the team feel about reusing issues for multiple PRs? I am re-creating this issue, and wondering do we normally create an issue per task or can one issue be represented in multiple PRs?~~

EDIT: Feel like I answered my own question looking at how these things are linked. One issue per PR seems to be the only real logical choice.

You can link multiple issues to one PR if needed ...sometimes a PR will close multiple open issues.

fbwoolf · 2024-09-24T23:19:31Z

@camerow are you using GH notifications? They can be super helpful. I keep a tab pinned with my notifications always open.

fbwoolf · 2024-09-24T23:24:46Z

packages/analytics/src/client/client.ts

+  }
+  // TODO: replace ledger_nativesegwit_add_nonwitnessutxo with this method call.
+  // TODO: replace ledger_nativesegwit_skip_add_nonwitnessutxo with this method call.
+  public async trackLedgerNativesegwitAddNonwitnessUtxo(properties: {


Suggested change

public async trackLedgerNativesegwitAddNonwitnessUtxo(properties: {

public async trackLedgerNativeSegwitAddNonWitnessUtxo(properties: {

fbwoolf · 2024-09-24T23:39:30Z

packages/analytics/src/client/client.ts

+
+  // TODO: replace view_transaction_confirmation with this method call.
+  public async trackViewTransactionConfirmation(properties: { symbol: string }) {
+    return this.track('Transaction Confirmation Viewed', properties);


Not sure about the naming pattern change here? @markmhendrickson is this change ok? I know we coordinated the snake_case for naming quite awhile ago.

I'm cool with updating the naming pattern. Let's just be sure to alias old names to the new ones if we update them, so we can continue reporting consistently.

Mentioned on slack, but would push back against this change. What's superior about this naming scheme that warrants the effort of doing the aliasing for 100s of events? Per Mixpanel docs

Generally, adopting snake_case for your event and property names tend to be more robust, especially if you plan to export your Mixpanel data to downstream processes such as data warehouses.

If we ever want to programmatically do anything with this data, we have to start escaping spaces in the ids etc, it's a limiting decision.

I see parallels here with how we're moving to ids for our localisation. sentences !== programatic identifiers.

fbwoolf · 2024-09-24T23:47:59Z

packages/analytics/src/client/client.ts

+  'defaultProperties' | 'defaultTraits'
+>;
+
+export class AnalyticsClient<T extends GenericAnalyticsClient> {


Why the choice here to use a class over a function? We just refactored our api clients away from using classes.

I would like to store a reference to options and creating a class seemed to be the best way. WHat is the disadvantage of a class over a function?

Nothing inherently wrong using a class here, and think it's okay for a package like this, but I think Fara's mentioning because we typically favour a more functional approach. Classes often lead to inheritance over composition, mutative methods over pure functions etc. Use of classes in Javascript is a hotly debated topic.

Yep, was just questioning based on Kyran's feedback here. Some suggest to avoid classes in JS if possible, so was just curious about your choice.

docs/extension-development.md

pete-watters

Nice work @camerow 👍

This code LGTM so good to merge, especially as the package is not yet consumed.

I like the readability of the uppercase events better but I'm not sure if we need snake_case for some reason

kyranjamie

Great start @camerow

packages/analytics/README.md

kyranjamie · 2024-09-25T07:34:30Z

packages/analytics/src/index.native.ts

+const configureAnalyticsClient = ({
+  writeKey,
+  defaultProperties,
+  client,
+}: AnalyticsClientConfig) => {
+  const nativeClient =
+    client ??
+    createClient({
+      writeKey,
+    });
+
+  analyticsClient = new AnalyticsClient(nativeClient, { defaultProperties });
+
+  return analyticsClient;
+};
+
+export { analyticsClient, configureAnalyticsClient };


Suggested change

const configureAnalyticsClient = ({

writeKey,

defaultProperties,

client,

}: AnalyticsClientConfig) => {

const nativeClient =

client ??

createClient({

writeKey,

});

analyticsClient = new AnalyticsClient(nativeClient, { defaultProperties });

return analyticsClient;

};

export { analyticsClient, configureAnalyticsClient };

export function configureAnalyticsClient ({

writeKey,

defaultProperties,

client,

}: AnalyticsClientConfig) {

const nativeClient =

client ??

createClient({

writeKey,

});

analyticsClient = new AnalyticsClient(nativeClient, { defaultProperties });

return analyticsClient;

}

Convention used elsewhere is to use function declarations

kyranjamie · 2024-09-25T07:35:15Z

packages/analytics/src/utils/types.shared.ts

+export type JsonValue = boolean | number | string | null | JsonList | JsonMap | undefined;
+
+export interface JsonMap {
+  [key: string]: JsonValue;
+  [index: number]: JsonValue;
+}
+
+export type JsonList = JsonValue[];


Seems like a candidate for utils pkg

edgarkhanzadian

Great work!

edgarkhanzadian · 2024-09-25T12:14:43Z

.tool-versions

@@ -0,0 +1 @@
+nodejs 20.12.2


As we already have .prototools, i would suggest not using more dot files to keep a version of tools.

camerow self-assigned this Sep 24, 2024

camerow requested review from fbwoolf, kyranjamie and pete-watters September 24, 2024 20:31

camerow force-pushed the refactor/segment-analytics branch from 5b20b24 to cd7e962 Compare September 24, 2024 20:46

camerow force-pushed the refactor/segment-analytics branch 2 times, most recently from c92b553 to 87dcb52 Compare September 24, 2024 21:21

refactor: add new analytics package, leather-io/issues#345

737d3fb

camerow force-pushed the refactor/segment-analytics branch from 87dcb52 to 737d3fb Compare September 24, 2024 21:23

fbwoolf reviewed Sep 24, 2024

View reviewed changes

pete-watters reviewed Sep 25, 2024

View reviewed changes

docs/extension-development.md Show resolved Hide resolved

pete-watters approved these changes Sep 25, 2024

View reviewed changes

kyranjamie reviewed Sep 25, 2024

View reviewed changes

edgarkhanzadian approved these changes Sep 25, 2024

View reviewed changes

camerow added 2 commits September 27, 2024 06:47

chore: clean up docs

d49c5ba

chore: pr feedback

143a8c5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create a simple event tracking package #472

Create a simple event tracking package #472

camerow commented Sep 24, 2024 •

edited

Loading

codecov bot commented Sep 24, 2024 •

edited

Loading

fbwoolf commented Sep 24, 2024

camerow commented Sep 24, 2024

camerow commented Sep 24, 2024 •

edited

Loading

fbwoolf commented Sep 24, 2024

fbwoolf commented Sep 24, 2024

fbwoolf Sep 24, 2024

fbwoolf Sep 24, 2024

markmhendrickson Sep 25, 2024

kyranjamie Sep 25, 2024 •

edited

Loading

fbwoolf Sep 24, 2024

camerow Sep 26, 2024

kyranjamie Sep 27, 2024

fbwoolf Sep 27, 2024

pete-watters left a comment

kyranjamie left a comment

kyranjamie Sep 25, 2024

kyranjamie Sep 25, 2024

edgarkhanzadian left a comment

edgarkhanzadian Sep 25, 2024

	public async trackLedgerNativesegwitAddNonwitnessUtxo(properties: {
	public async trackLedgerNativeSegwitAddNonWitnessUtxo(properties: {

Create a simple event tracking package #472

Are you sure you want to change the base?

Create a simple event tracking package #472

Conversation

camerow commented Sep 24, 2024 • edited Loading

Overview

Assumptions

Future steps

codecov bot commented Sep 24, 2024 • edited Loading

Codecov Report

fbwoolf commented Sep 24, 2024

camerow commented Sep 24, 2024

camerow commented Sep 24, 2024 • edited Loading

fbwoolf commented Sep 24, 2024

fbwoolf commented Sep 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyranjamie Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pete-watters left a comment

Choose a reason for hiding this comment

kyranjamie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edgarkhanzadian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camerow commented Sep 24, 2024 •

edited

Loading

codecov bot commented Sep 24, 2024 •

edited

Loading

camerow commented Sep 24, 2024 •

edited

Loading

kyranjamie Sep 25, 2024 •

edited

Loading