add usage tracking (with segment) #174

cgardens · 2020-09-04T22:40:46Z

What

Add TrackingStrategy to the env.
Add customerId to the workspace config so that we can group the actions for an instance.
- this comes with a hack to make sure it is set in time. I'll add a comment below that people can use to leave feedback.
Add TrackingClients for tracking usage. (iface, SegmentTrackingClient, and LoggingTrackingTrackingClient)
Use tracking client as a singleton (so that we do not need to dependency inject it everywhere). This may be controversial and would like to hear if anyone doesn't like this choice. I'll add a comment below that people can use to leave feedback.
Added an example usage of how to use the tracking client (but have not added all of the track events that we want yet). Example usages are in ServerApp.java and SchedulerHandler.java

Checklist (going to hold off on this until I get some initial feedback that people are happy with the approach)

Set this up in segment and make sure it reports properly
Add env variable for tracking to docker configs
Unit tests
Add track invocations for everything we actually want to track

Recommended reading order

TrackingClient.java, SegmentTrackingClient.java, LoggingTrackingTrackingClient.java, TrackingClientSingleton.java
EnvConfigs.java
SeverApp.java
the rest

cgardens · 2020-09-04T22:42:16Z

dataline-analytics/src/main/java/io/dataline/analytics/TrackingClientSingleton.java

+
+  private static TrackingClient trackingClient;
+
+  public static TrackingClient get() {


I'd like to avoid having to dependency inject the tracker everywhere. Would be curious if people are okay with this approach for tracking.

I'm fine with the production tracker being a singleton but it seems like it needs to be injectable if we're going to test our metrics? And if we don't test our metrics we can't really trust them 100%.

per our conversation offline, keeping this as a singleton is valuable so that we don't have to dependency inject everywhere. but in cases where we do need to test the behavior, we might dependency inject more locally into a class.

Agreed for the singleton.

We could have a way to change it with a setter so you can actually set it to a mock and test interactions in the test.

I would suggest having it set by default to Logging and have a way to set it up with env that you call in the main. Something like a TrackingClientSingleton.configure(opts).

WDYT?

@michel-tricot - i took a hack at this. not quite what you described. lmk if this works for you.

Let's talk about it offline. I am not fan that you are creating a configpersistence that could be different from the rest of the application.

We shouldn't be afraid to have a static initialializer that needs to be called in the main (so you can inject options). Having the initializer would probably also solve the TrackingIdentiy supplier piece

cgardens · 2020-09-04T22:46:15Z

dataline-server/src/main/java/io/dataline/server/ServerApp.java

@@ -59,6 +69,12 @@ public ServerApp(final Path configRoot) {
  }

  public void start() throws Exception {
+    // hack: upon installation we need to assign a random customerId so that when
+    // tracking we can associate all action with the correct anonymous id.
+    setCustomerIdIfNotSet(configRoot);


This is really not great. I like it better than the other option I could come up with though. The alternative I think is to have an initialize script in our docker setup that finds the default workspace json file and mutates it to inject a customerId. I don't like it as much as what I have here for two reasons:

Don't want to add initialize scripts in docker right now since we don't do this for anything else.

It's a hack but at least it's pretty easy to find and understand when it's in the java code as opposed to some random bash script.

jrhizor

Some small comments.

Additionally, we should change customerId to instanceId/installationId/similar since most of our users will not actually be customers at first.

jrhizor · 2020-09-04T22:48:08Z

dataline-config/models/src/main/java/io/dataline/config/EnvConfigs.java

@@ -83,6 +86,22 @@ public String getDockerNetwork() {
    return DEFAULT_NETWORK;
  }

+  @Override
+  public TrackingStrategy getTrackingStrategy() {


Why not use the enum's valueOf with a fallback to the default?

dataline-analytics/build.gradle

jrhizor · 2020-09-04T22:59:34Z

dataline-analytics/src/main/java/io/dataline/analytics/SegmentTrackingClient.java

+
+public class SegmentTrackingClient implements TrackingClient {
+
+  private static final String SEGMENT_WRITE_KEY = "123";


Will this be hardcoded with the actual key?

i was planning to. it's not considered a secret. i don't see alot of value in making it an env variable. will have to dependency inject it so that i can test this class but planning to keep it hardcoded.

jrhizor · 2020-09-04T23:02:57Z

dataline-analytics/src/main/java/io/dataline/analytics/SegmentTrackingClient.java

+public class SegmentTrackingClient implements TrackingClient {
+
+  private static final String SEGMENT_WRITE_KEY = "123";
+  private static final String EMAIL_KEY = "email";


nit: I'd be fine inlining this. Two adjacent *_KEY definitions with different meanings makes you do a double take.

jrhizor · 2020-09-04T23:04:38Z

dataline-analytics/src/main/java/io/dataline/analytics/TrackingClientSingleton.java

+
+  private static TrackingClient trackingClient;
+
+  public static TrackingClient get() {


I'm fine with the production tracker being a singleton but it seems like it needs to be injectable if we're going to test our metrics? And if we don't test our metrics we can't really trust them 100%.

jrhizor · 2020-09-04T23:06:03Z

dataline-analytics/src/main/java/io/dataline/analytics/TrackingClientSingleton.java

+        case SEGMENT:
+          trackingClient = new SegmentTrackingClient(identitySupplier);
+          break;
+        case LOGGING:


When would people want to configure logging metrics instead of segment metrics?

any sort of local development.

michel-tricot · 2020-09-05T06:29:49Z

dataline-analytics/src/main/java/io/dataline/analytics/TrackingClientSingleton.java

+
+  private static TrackingClient trackingClient;
+
+  public static TrackingClient get() {


Agreed for the singleton.

We could have a way to change it with a setter so you can actually set it to a mock and test interactions in the test.

michel-tricot · 2020-09-05T06:31:26Z

dataline-analytics/src/main/java/io/dataline/analytics/TrackingClientSingleton.java

+
+  private static TrackingClient trackingClient;
+
+  public static TrackingClient get() {


I would suggest having it set by default to Logging and have a way to set it up with env that you call in the main. Something like a TrackingClientSingleton.configure(opts).

WDYT?

michel-tricot · 2020-09-05T06:49:33Z

dataline-analytics/src/main/java/io/dataline/analytics/TrackingClientSingleton.java

+    final Configs configs = new EnvConfigs();
+    final Path configRoot = configs.getConfigRoot();
+
+    final StandardWorkspace workspace;


can you put the decl with the intialization?

michel-tricot · 2020-09-05T06:50:34Z

dataline-analytics/src/main/java/io/dataline/analytics/TrackingClientSingleton.java

+  public static TrackingClient get() {
+    if (trackingClient == null) {
+      final TrackingIdentity trackingIdentity = getTrackingIdentity();
+      final Supplier<TrackingIdentity> identitySupplier = () -> trackingIdentity;


Why do you need to pass a supplier instead of the identity directly?

Wouldn't the TrackingIdentity change between the time of install and after they entered their email? Maybe the supplier should just be getTrackingIdentity

michel-tricot · 2020-09-05T06:53:55Z

dataline-config/models/src/main/java/io/dataline/config/EnvConfigs.java

@@ -37,6 +37,9 @@
  public static final String WORKSPACE_DOCKER_MOUNT = "WORKSPACE_DOCKER_MOUNT";
  public static final String CONFIG_ROOT = "CONFIG_ROOT";
  public static final String DOCKER_NETWORK = "DOCKER_NETWORK";
+  public static final String TRACKING_STRATEGY = "TRACKING_STRATEGY";
+  public static final String TRACKING_STRATEGY_SEGMENT = "segment";


These finals are not necessary you should be able to use the enum directly (with value of and potentially a case change)

michel-tricot · 2020-09-05T06:54:09Z

dataline-config/models/src/main/java/io/dataline/config/EnvConfigs.java

@@ -83,6 +86,22 @@ public String getDockerNetwork() {
    return DEFAULT_NETWORK;
  }

+  @Override
+  public TrackingStrategy getTrackingStrategy() {


michel-tricot · 2020-09-05T06:56:20Z

dataline-server/src/main/java/io/dataline/server/ServerApp.java

@@ -115,6 +131,32 @@ public void configure() {
    server.join();
  }

+  private void setCustomerIdIfNotSet(final Path configRoot) {


Shouldn't that code live in TrackingClientSingleton ?

interesting thought. if we only ever want to use customerId in the tracker, then i'd be fine with that. in the current iteration of the PR, i've added customerId as part of the API interface in the WorkspaceRead struct. If we think customerId is useful outside of just tracking and should stay in the API interface, then I think it's better for it to be in ServerApp than for it to be in TrackingClientSingleton, because then the ServerApp and its data model would have an implicit dependency TrackingClientSingleton being initialized. If we want to confine customerId just to tracking and remove it from the API, then yeah, happy to stick this in TrackingClientSingleton.

Ok let's keep that as a hack then.

michel-tricot · 2020-09-09T03:01:34Z

dataline-analytics/src/main/java/io/dataline/analytics/TrackingClientSingleton.java

+import io.dataline.config.persistence.DefaultConfigPersistence;
+import java.nio.file.Path;
+
+public class TrackingClientSingleton {


btw this singleton is not thread safe.

okay i'll take a look

michel-tricot · 2020-09-09T03:11:45Z

dataline-analytics/src/main/java/io/dataline/analytics/TrackingClientSingleton.java

+
+  private static TrackingClient trackingClient;
+
+  public static TrackingClient get() {


Let's talk about it offline. I am not fan that you are creating a configpersistence that could be different from the rest of the application.

We shouldn't be afraid to have a static initialializer that needs to be called in the main (so you can inject options). Having the initializer would probably also solve the TrackingIdentiy supplier piece

michel-tricot · 2020-09-09T03:12:55Z

dataline-analytics/src/main/java/io/dataline/analytics/TrackingIdentitySupplier.java

+  public TrackingIdentity get() {
+    try {
+      final StandardWorkspace workspace = configPersistence.getConfig(
+          ConfigSchema.STANDARD_WORKSPACE,


Random question: how is it going to work when we have multiple workspaces?

It almost feels like the workspace should be an attribute of the tracking instead of being the key. and the user/email/anonymous id should tied to the application itself

i think we cross that bridge when we have multiple workspaces. it's unclear at this point if that's even a thing we'll end up needing.

michel-tricot · 2020-09-09T03:16:20Z

dataline-analytics/src/main/java/io/dataline/analytics/TrackingIdentitySupplier.java

+        email = workspace.getEmail();
+      }
+      return new TrackingIdentity(workspace.getCustomerId(), email);
+    } catch (ConfigNotFoundException e) {


you can collapse these exceptions since the confignotfound already mentions a similar message

michel-tricot · 2020-09-09T03:20:26Z

I don't want to block the merge either so go ahead, we can always refactor later

cgardens · 2020-09-09T15:26:23Z

i'll take another look at this today and address the issues you mentioned.

fix dep syntax valueOf, envconfigs test inline add segment write key remove supplier tracking identity supplier test add TrackingClientSingletonTest

* #174 source file: fix csv schema discovery * #174 source file: upd changelog * auto-bump connector version [ci skip] Co-authored-by: Octavia Squidington III <[email protected]>

cgardens commented Sep 4, 2020

View reviewed changes

cgardens requested review from michel-tricot, jrhizor and sherifnada September 4, 2020 22:47

jrhizor approved these changes Sep 4, 2020

View reviewed changes

michel-tricot reviewed Sep 5, 2020

View reviewed changes

michel-tricot approved these changes Sep 7, 2020

View reviewed changes

michel-tricot reviewed Sep 9, 2020

View reviewed changes

add usage tracking

f931491

fix dep syntax valueOf, envconfigs test inline add segment write key remove supplier tracking identity supplier test add TrackingClientSingletonTest

cgardens force-pushed the cgardens/add_usage_tracking branch from e07c40b to f931491 Compare September 9, 2020 17:05

cgardens added 6 commits September 9, 2020 14:37

progress

d75b22a

supplier; tests

0bf610e

add other traits to identify call

575469f

tracking strategy to env

f2e9cfd

clean up

797b7a3

whitespace

541eb58

cgardens merged commit 38e84f1 into master Sep 9, 2020

cgardens deleted the cgardens/add_usage_tracking branch September 9, 2020 23:36

cgardens mentioned this pull request Sep 14, 2020

Track customer usage on the backend #128

Closed

davydov-d added a commit that referenced this pull request Aug 23, 2022

#174 source file: fix csv schema discovery

590f829

davydov-d added a commit that referenced this pull request Aug 23, 2022

#174 source file: upd changelog

a973b35


		private static TrackingClient trackingClient;

		public static TrackingClient get() {


		public class SegmentTrackingClient implements TrackingClient {

		private static final String SEGMENT_WRITE_KEY = "123";

add usage tracking (with segment) #174

add usage tracking (with segment) #174

Conversation

cgardens commented Sep 4, 2020 • edited Loading

What

Checklist (going to hold off on this until I get some initial feedback that people are happy with the approach)

Recommended reading order

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

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

jrhizor left a 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgardens Sep 5, 2020 • 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

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

michel-tricot commented Sep 9, 2020

cgardens commented Sep 9, 2020

cgardens commented Sep 4, 2020 •

edited

Loading

cgardens Sep 5, 2020 •

edited

Loading