Add source to analytics events. #2750

mitchell-as · 2023-09-12T17:39:19Z

DX-2145 We identify State Tool traffic as State Tool traffic

This allows for differentiation between state tool, offline installer, executor, etc.

This allows for differentiation between state tool, offline installer, executors, etc.

mitchell-as · 2023-09-12T18:23:59Z

cmd/state-installer/cmd.go

-const AnalyticsCat = "installer"
-const AnalyticsFunnelCat = "installer-funnel"


Moved into analytics/constants.go

Naatan

I think we should set this at construction time. We would never want to modify this on a per-event basis.

Also note with the svc that it handles two types of analytics:

First, it has its own analytics that it sets up here:
- cli/cmd/state-svc/main.go
  
  Line 101 in 4087dc2
  
  an := anaSync.New(cfg, auth, out)
- This one should have the state-svc as the source
It also facilitates analytics for the state tool itself, this one is constructed here:
- cli/cmd/state-svc/internal/resolver/resolver.go
  
  Line 76 in 4087dc2
  
  anForClient := sync.New(cfg, auth, nil)
- This one should have the state tool as the source.

Also improve forwarding events on behalf of executors.

mitchell-as · 2023-09-13T16:48:49Z

cmd/state-svc/internal/rtwatcher/watcher.go

@@ -97,7 +97,7 @@ func (w *Watcher) check() {

 func (w *Watcher) RecordUsage(e entry) {
 	logging.Debug("Recording usage of %s (%d)", e.Exec, e.PID)
-	w.an.Event(anaConst.CatRuntimeUsage, anaConst.ActRuntimeHeartbeat, e.Dims)
+	w.an.EventWithSource(anaConst.CatRuntimeUsage, anaConst.ActRuntimeHeartbeat, anaConst.SrcExecutor, e.Dims)


It looks to me like all heartbeats come from executors, so that's why the source is this and not State Tool.

Good call! I forgot about that caveat.

mitchell-as · 2023-09-13T16:51:38Z

Good call with putting source in the analytics client. We still need some wiggle room to differentiate between state tool and executors (particularly in the runtime area), so state-svc cannot assume anForClient is always coming from State Tool, and instead forwards on behalf of the incoming source (currently State Tool or Executor).

If you don't care to differentiate, let me know and I'll drop all that special handling. Or if you have another idea, let me know.

Naatan

Looks great! In addition to the one case I flagged for executor handling, I think we should also have our analytics integration tests specifically check for it.

IIRC the analytics tests are already triggering state tool events as well as executor events, so with the data we're already generating we could assert that they have the expected source set.

pkg/platform/runtime/analytics/analytics.go

…integration tests.

Naatan · 2023-09-14T17:23:32Z

pkg/platform/runtime/analytics/analytics.go

 )

 var isExecutor bool

 func init() {
-	if osutils.Executable() == constants.StateExecutorCmd {
-		isExecutor = true
+	if isExec, err := executors.IsExecutor(osutils.Executable()); err == nil {


As I said executors do not use the analytics package directly, so I don't think we need this?

Executors can trigger runtime setup/use, right? That will send out analytics requests to state-svc, which should then forward them to GA: https://github.com/ActiveState/cli/pull/2750/files#diff-d6507bff98aa2549ce60b737f856f84e7a24e20bf39fef4a7ba6f05adf62ffe7R100-R103

Or am I missing something? When I checked the analytics int test event files, I'm seeing runtime-use:attempt, runtime-debug:download, and runtime-debug:success events for executors (e.g. python --version). This is what I expected to see.

That's right, but this code is checking the current executable. There is never a scenario in which an executor invokes the analytics code (whether sync or async) directly. Instead they talk directly to the svc without the analytics library. The svc then uses its analytics library to report the event.

In other words; I don't believe there is a scenario in which this conditional evaluates to true.

Okay, you're right. I went through a debugger and that code does not appear to be hit. I've removed it.

Naatan · 2023-09-14T17:27:15Z

test/integration/analytics_int_test.go

 		fmt.Sprintf("output:\n%s\n%s",
 			cp.Snapshot(), ts.DebugLogs()))

-	heartbeatInitialCount := countEvents(events, anaConst.CatRuntimeUsage, anaConst.ActRuntimeHeartbeat)
+	heartbeatInitialCount := countEvents(events, anaConst.CatRuntimeUsage, anaConst.ActRuntimeHeartbeat, anaConst.SrcExecutor)


Can we also assert here that we got both executor and state tool heartbeats?

ie. the state activate should've triggered heartbeats from the state tool, whereas the executor would've triggered it from the executor.

Okay, so my comment here is not valid, right? #2750 (comment) I want to confirm before I spend time on this.

Sorry that's my bad. Your comment there is indeed incorrect, I should've known that but I guess I wasn't thinking it through.

Naatan

See comments.

Executors should not invoke this code directly, so there should be no analytics source confusion.

Naatan

Looks good! I think you missed this comment though: #2750 (comment)

mitchell-as · 2023-09-15T14:29:43Z

Sorry about that. Too many double negatives. I got confused and thought the opposite of what you were thinking :S

Naatan · 2023-09-15T16:04:31Z

cmd/state-svc/internal/rtwatcher/watcher.go

+	source := anaConst.SrcExecutor
+	if filepath.Base(e.Exec) == constants.StateCmd+osutils.ExeExt {
+		source = anaConst.SrcStateTool
+	}
+	w.an.EventWithSource(anaConst.CatRuntimeUsage, anaConst.ActRuntimeHeartbeat, source, e.Dims)


This feels too error prone to me. Could we instead add the source as an argument for the ReportRuntimeUsage resolver on the svc?

The only place executors call this is here:

cli/internal/svcctl/comm.go

Line 142 in be95799

_, err = resolver.ReportRuntimeUsage(context.Background(), pidNum, hb.ExecPath, dimsJSON)

Any other place would be state tool. I'd be fine defaulting to state tool if the source is empty.

mitchell-as · 2023-09-15T16:59:49Z

The change got a bit involved since there's graphql involved, so let me know if you had something else in mind.

Naatan

Looks great!

mitchell-as added the Test: analytics label Sep 12, 2023

mitchell-as force-pushed the mitchell/dx-2145 branch from 778f37a to 84adcdc Compare September 12, 2023 18:02

Add source to analytics events.

44abffa

This allows for differentiation between state tool, offline installer, executors, etc.

mitchell-as force-pushed the mitchell/dx-2145 branch from 84adcdc to 44abffa Compare September 12, 2023 18:19

mitchell-as commented Sep 12, 2023

View reviewed changes

mitchell-as requested a review from Naatan September 12, 2023 18:31

mitchell-as marked this pull request as ready for review September 12, 2023 18:31

Naatan requested changes Sep 12, 2023

View reviewed changes

Move analytics source into clients.

1a6ad98

Also improve forwarding events on behalf of executors.

mitchell-as force-pushed the mitchell/dx-2145 branch from fa538a8 to 1a6ad98 Compare September 13, 2023 16:44

mitchell-as commented Sep 13, 2023

View reviewed changes

mitchell-as requested a review from Naatan September 13, 2023 16:58

Naatan requested changes Sep 13, 2023

View reviewed changes

pkg/platform/runtime/analytics/analytics.go Outdated Show resolved Hide resolved

mitchell-as force-pushed the mitchell/dx-2145 branch from 0e09d14 to 52b7462 Compare September 13, 2023 20:35

Attempt to fix executor recognition for analytics source and updated …

e90f8ea

…integration tests.

mitchell-as force-pushed the mitchell/dx-2145 branch from 52b7462 to e90f8ea Compare September 13, 2023 21:51

mitchell-as requested a review from Naatan September 13, 2023 21:59

Naatan requested changes Sep 14, 2023

View reviewed changes

mitchell-as requested a review from Naatan September 14, 2023 18:18

Naatan requested changes Sep 14, 2023

View reviewed changes

Remove special analytics source handling from runtime.

7c7ae9c

Executors should not invoke this code directly, so there should be no analytics source confusion.

mitchell-as requested a review from Naatan September 14, 2023 19:06

Naatan requested changes Sep 14, 2023

View reviewed changes

mitchell-as force-pushed the mitchell/dx-2145 branch from edd93cc to 5ab86e5 Compare September 15, 2023 14:42

State Tool can emit heartbeats too, not just executors.

a6928da

mitchell-as force-pushed the mitchell/dx-2145 branch from 5ab86e5 to a6928da Compare September 15, 2023 14:50

mitchell-as requested a review from Naatan September 15, 2023 15:13

Naatan requested changes Sep 15, 2023

View reviewed changes

mitchell-as force-pushed the mitchell/dx-2145 branch from 82a5165 to 85c050e Compare September 15, 2023 16:28

Heartbeats should contain their source.

bfef23d

mitchell-as force-pushed the mitchell/dx-2145 branch from 85c050e to bfef23d Compare September 15, 2023 16:48

mitchell-as requested a review from Naatan September 15, 2023 16:59

Naatan approved these changes Sep 15, 2023

View reviewed changes

mitchell-as merged commit 1335957 into version/0-41-0-RC1 Sep 15, 2023
6 checks passed

mitchell-as deleted the mitchell/dx-2145 branch September 15, 2023 17:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add source to analytics events. #2750

Add source to analytics events. #2750

mitchell-as commented Sep 12, 2023 •

edited by github-actions bot

Loading

mitchell-as Sep 12, 2023

Naatan left a comment

mitchell-as Sep 13, 2023

Naatan Sep 13, 2023

mitchell-as commented Sep 13, 2023

Naatan left a comment

Naatan Sep 14, 2023

mitchell-as Sep 14, 2023 •

edited

Loading

Naatan Sep 14, 2023

mitchell-as Sep 14, 2023

Naatan Sep 14, 2023

mitchell-as Sep 14, 2023 •

edited

Loading

Naatan Sep 14, 2023

Naatan left a comment

Naatan left a comment

mitchell-as commented Sep 15, 2023

Naatan Sep 15, 2023

mitchell-as commented Sep 15, 2023

Naatan left a comment

		const AnalyticsCat = "installer"
		const AnalyticsFunnelCat = "installer-funnel"

Add source to analytics events. #2750

Add source to analytics events. #2750

Conversation

mitchell-as commented Sep 12, 2023 • edited by github-actions bot Loading

Choose a reason for hiding this comment

Naatan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchell-as commented Sep 13, 2023

Naatan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchell-as Sep 14, 2023 • 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

mitchell-as Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Naatan left a comment

Choose a reason for hiding this comment

Naatan left a comment

Choose a reason for hiding this comment

mitchell-as commented Sep 15, 2023

Choose a reason for hiding this comment

mitchell-as commented Sep 15, 2023

Naatan left a comment

Choose a reason for hiding this comment

mitchell-as commented Sep 12, 2023 •

edited by github-actions bot

Loading

mitchell-as Sep 14, 2023 •

edited

Loading

mitchell-as Sep 14, 2023 •

edited

Loading