Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add source to analytics events. #2750

Merged
merged 6 commits into from
Sep 15, 2023
Merged

Add source to analytics events. #2750

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
44abffa
Add source to analytics events.
mitchell-as Sep 12, 2023
1a6ad98
Move analytics source into clients.
mitchell-as Sep 13, 2023
e90f8ea
Attempt to fix executor recognition for analytics source and updated …
mitchell-as Sep 13, 2023
7c7ae9c
Remove special analytics source handling from runtime.
mitchell-as Sep 14, 2023
a6928da
State Tool can emit heartbeats too, not just executors.
mitchell-as Sep 15, 2023
bfef23d
Heartbeats should contain their source.
mitchell-as Sep 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions pkg/platform/runtime/analytics/analytics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,15 @@ import (
"github.com/ActiveState/cli/internal/analytics"
anaConsts "github.com/ActiveState/cli/internal/analytics/constants"
"github.com/ActiveState/cli/internal/analytics/dimensions"
"github.com/ActiveState/cli/internal/constants"
"github.com/ActiveState/cli/internal/osutils"
"github.com/ActiveState/cli/pkg/platform/runtime/executors"
)

var isExecutor bool

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@mitchell-as mitchell-as Sep 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

isExecutor = isExec
}
}

Expand Down
42 changes: 21 additions & 21 deletions test/integration/analytics_int_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,16 +82,16 @@ func (suite *AnalyticsIntegrationTestSuite) TestActivateEvents() {
suite.Require().NotEmpty(events)

// Runtime:start events
suite.assertNEvents(events, 1, anaConst.CatRuntime, anaConst.ActRuntimeStart,
suite.assertNEvents(events, 1, anaConst.CatRuntime, anaConst.ActRuntimeStart, anaConst.SrcStateTool,
fmt.Sprintf("output:\n%s\n%s",
cp.Snapshot(), ts.DebugLogs()))

// Runtime:success events
suite.assertNEvents(events, 1, anaConst.CatRuntime, anaConst.ActRuntimeSuccess,
suite.assertNEvents(events, 1, anaConst.CatRuntime, anaConst.ActRuntimeSuccess, anaConst.SrcStateTool,
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@mitchell-as mitchell-as Sep 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if heartbeatInitialCount < 2 {
// It's possible due to the timing of the heartbeats and the fact that they are async that we have gotten either
// one or two by this point. Technically more is possible, just very unlikely.
Expand All @@ -104,7 +104,7 @@ func (suite *AnalyticsIntegrationTestSuite) TestActivateEvents() {
suite.Require().NotEmpty(events)

// Runtime-use:heartbeat events - should now be at least +1 because we waited <heartbeatInterval>
suite.assertGtEvents(events, heartbeatInitialCount, anaConst.CatRuntimeUsage, anaConst.ActRuntimeHeartbeat,
suite.assertGtEvents(events, heartbeatInitialCount, anaConst.CatRuntimeUsage, anaConst.ActRuntimeHeartbeat, anaConst.SrcExecutor,
fmt.Sprintf("output:\n%s\n%s",
cp.Snapshot(), ts.DebugLogs()))

Expand All @@ -121,11 +121,11 @@ func (suite *AnalyticsIntegrationTestSuite) TestActivateEvents() {
}
return (*e.Dimensions.Trigger) == target.TriggerExecutor.String()
})
suite.Require().Equal(1, countEvents(executorEvents, anaConst.CatRuntimeUsage, anaConst.ActRuntimeAttempt),
suite.Require().Equal(1, countEvents(executorEvents, anaConst.CatRuntimeUsage, anaConst.ActRuntimeAttempt, anaConst.SrcExecutor),
ts.DebugMessage("Should have a runtime attempt, events:\n"+debugEvents(suite.T(), executorEvents)))
// It's possible due to the timing of the heartbeats and the fact that they are async that we have gotten either
// one or two by this point. Technically more is possible, just very unlikely.
numHeartbeats := countEvents(executorEvents, anaConst.CatRuntimeUsage, anaConst.ActRuntimeHeartbeat)
numHeartbeats := countEvents(executorEvents, anaConst.CatRuntimeUsage, anaConst.ActRuntimeHeartbeat, anaConst.SrcExecutor)
suite.Require().Greater(numHeartbeats, 0, "Should have a heartbeat")
suite.Require().LessOrEqual(numHeartbeats, 2, "Should not have excessive heartbeats")
var heartbeatEvent *reporters.TestLogEntry
Expand Down Expand Up @@ -153,13 +153,13 @@ func (suite *AnalyticsIntegrationTestSuite) TestActivateEvents() {

events = parseAnalyticsEvents(suite, ts)
suite.Require().NotEmpty(events)
eventsAfterExit := countEvents(events, anaConst.CatRuntimeUsage, anaConst.ActRuntimeHeartbeat)
eventsAfterExit := countEvents(events, anaConst.CatRuntimeUsage, anaConst.ActRuntimeHeartbeat, anaConst.SrcExecutor)

time.Sleep(sleepTime)

eventsAfter := parseAnalyticsEvents(suite, ts)
suite.Require().NotEmpty(eventsAfter)
eventsAfterWait := countEvents(eventsAfter, anaConst.CatRuntimeUsage, anaConst.ActRuntimeHeartbeat)
eventsAfterWait := countEvents(eventsAfter, anaConst.CatRuntimeUsage, anaConst.ActRuntimeHeartbeat, anaConst.SrcExecutor)

suite.Equal(eventsAfterExit, eventsAfterWait,
fmt.Sprintf("Heartbeats should stop ticking after exiting subshell.\n"+
Expand All @@ -177,9 +177,9 @@ func (suite *AnalyticsIntegrationTestSuite) TestActivateEvents() {
suite.assertSequentialEvents(events)
}

func countEvents(events []reporters.TestLogEntry, category, action string) int {
func countEvents(events []reporters.TestLogEntry, category, action, source string) int {
filteredEvents := funk.Filter(events, func(e reporters.TestLogEntry) bool {
return e.Category == category && e.Action == action && e.Source != ""
return e.Category == category && e.Action == action && e.Source == source
}).([]reporters.TestLogEntry)
return len(filteredEvents)
}
Expand All @@ -203,15 +203,15 @@ func filterEvents(events []reporters.TestLogEntry, filters ...func(e reporters.T
}

func (suite *AnalyticsIntegrationTestSuite) assertNEvents(events []reporters.TestLogEntry,
expectedN int, category, action string, errMsg string) {
suite.Assert().Equal(expectedN, countEvents(events, category, action),
expectedN int, category, action, source string, errMsg string) {
suite.Assert().Equal(expectedN, countEvents(events, category, action, source),
"Expected %d %s:%s events.\nFile location: %s\nEvents received:\n%s\nError:\n%s",
expectedN, category, action, suite.eventsfile, suite.summarizeEvents(events), errMsg)
}

func (suite *AnalyticsIntegrationTestSuite) assertGtEvents(events []reporters.TestLogEntry,
greaterThanN int, category, action string, errMsg string) {
suite.Assert().Greater(countEvents(events, category, action), greaterThanN,
greaterThanN int, category, action, source string, errMsg string) {
suite.Assert().Greater(countEvents(events, category, action, source), greaterThanN,
fmt.Sprintf("Expected more than %d %s:%s events.\nFile location: %s\nEvents received:\n%s\nError:\n%s",
greaterThanN, category, action, suite.eventsfile, suite.summarizeEvents(events), errMsg))
}
Expand Down Expand Up @@ -254,16 +254,16 @@ func (suite *AnalyticsIntegrationTestSuite) assertSequentialEvents(events []repo
func (suite *AnalyticsIntegrationTestSuite) summarizeEvents(events []reporters.TestLogEntry) string {
summary := []string{}
for _, event := range events {
summary = append(summary, fmt.Sprintf("%s:%s:%s", event.Category, event.Action, event.Label))
summary = append(summary, fmt.Sprintf("%s:%s:%s (%s)", event.Category, event.Action, event.Label, event.Source))
}
return strings.Join(summary, "\n")
}

func (suite *AnalyticsIntegrationTestSuite) summarizeEventSequence(events []reporters.TestLogEntry) string {
summary := []string{}
for _, event := range events {
summary = append(summary, fmt.Sprintf("%s:%s:%s (seq: %s:%s:%d)\n",
event.Category, event.Action, event.Label,
summary = append(summary, fmt.Sprintf("%s:%s:%s (%s seq: %s:%s:%d)\n",
event.Category, event.Action, event.Label, event.Source,
*event.Dimensions.Command, (*event.Dimensions.InstanceID)[0:6], *event.Dimensions.Sequence))
}
return strings.Join(summary, "\n")
Expand Down Expand Up @@ -423,7 +423,7 @@ func (suite *AnalyticsIntegrationTestSuite) TestInputError() {
events := parseAnalyticsEvents(suite, ts)
suite.assertSequentialEvents(events)

suite.assertNEvents(events, 1, anaConst.CatDebug, anaConst.ActCommandInputError,
suite.assertNEvents(events, 1, anaConst.CatDebug, anaConst.ActCommandInputError, anaConst.SrcStateTool,
fmt.Sprintf("output:\n%s\n%s",
cp.Snapshot(), ts.DebugLogs()))

Expand Down Expand Up @@ -466,7 +466,7 @@ func (suite *AnalyticsIntegrationTestSuite) TestAttempts() {
for _, e := range events {
if strings.Contains(e.Category, "runtime") && strings.Contains(e.Action, "attempt") {
foundAttempts++
if strings.Contains(*e.Dimensions.Trigger, "exec") {
if strings.Contains(*e.Dimensions.Trigger, "exec") && strings.Contains(e.Source, anaConst.SrcExecutor) {
foundExecs++
}
}
Expand Down Expand Up @@ -564,8 +564,8 @@ func (suite *AnalyticsIntegrationTestSuite) TestConfigEvents() {
suite.Fail("Should find multiple config events")
}

suite.assertNEvents(events, 1, anaConst.CatConfig, anaConst.ActConfigSet, "Should be at one config set event")
suite.assertNEvents(events, 1, anaConst.CatConfig, anaConst.ActConfigUnset, "Should be at one config unset event")
suite.assertNEvents(events, 1, anaConst.CatConfig, anaConst.ActConfigSet, anaConst.SrcStateTool, "Should be at one config set event")
suite.assertNEvents(events, 1, anaConst.CatConfig, anaConst.ActConfigUnset, anaConst.SrcStateTool, "Should be at one config unset event")
suite.assertSequentialEvents(events)
}

Expand Down
4 changes: 2 additions & 2 deletions test/integration/offinstall_int_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ func (suite *OffInstallIntegrationTestSuite) TestInstallAndUninstall() {
heartbeat := suite.filterEvent(events, anaConst.CatRuntimeUsage, anaConst.ActRuntimeHeartbeat)
suite.assertDimensions(heartbeat)

nDelete := countEvents(events, anaConst.CatRuntimeUsage, anaConst.ActRuntimeDelete)
nDelete := countEvents(events, anaConst.CatRuntimeUsage, anaConst.ActRuntimeDelete, anaConst.SrcOfflineInstaller)
if nDelete != 0 {
suite.FailNow(fmt.Sprintf("Expected 0 delete events, got %d, events:\n%#v", nDelete, events))
}
Expand Down Expand Up @@ -147,7 +147,7 @@ func (suite *OffInstallIntegrationTestSuite) TestInstallAndUninstall() {
// Verify that our analytics event was fired
events := parseAnalyticsEvents(suite, ts)
suite.Require().NotEmpty(events)
nHeartbeat := countEvents(events, anaConst.CatRuntimeUsage, anaConst.ActRuntimeHeartbeat)
nHeartbeat := countEvents(events, anaConst.CatRuntimeUsage, anaConst.ActRuntimeHeartbeat, anaConst.SrcExecutor)
if nHeartbeat != 1 {
suite.FailNow(fmt.Sprintf("Expected 1 heartbeat event, got %d, events:\n%#v", nHeartbeat, events))
}
Expand Down
Loading