Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
1 change: 1 addition & 0 deletions internal/analytics/analytics.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,5 @@ type Dispatcher interface {

func init() {
configMediator.RegisterOption(constants.ReportAnalyticsConfig, configMediator.Bool, true)
configMediator.RegisterOption(constants.AnalyticsPixelOverrideConfig, configMediator.String, "")
}
7 changes: 4 additions & 3 deletions internal/analytics/client/sync/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -120,15 +120,16 @@ func New(source string, cfg *config.Instance, auth *authentication.Auth, out out

a.customDimensions = customDimensions

url := a.cfg.GetString(constants.AnalyticsPixelOverrideConfig)
// Register reporters
if condition.InTest() {
logging.Debug("Using test reporter")
a.NewReporter(reporters.NewTestReporter(reporters.TestReportFilepath()))
a.NewReporter(reporters.NewTestReporter(reporters.TestReportFilepath(), url))
logging.Debug("Using test reporter as instructed by env")
} else if v := os.Getenv(constants.AnalyticsLogEnvVarName); v != "" {
a.NewReporter(reporters.NewTestReporter(v))
a.NewReporter(reporters.NewTestReporter(v, url))
} else {
a.NewReporter(reporters.NewPixelReporter())
a.NewReporter(reporters.NewPixelReporter(url))
}

return a
Expand Down
10 changes: 7 additions & 3 deletions internal/analytics/client/sync/reporters/pixel.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,17 @@ type PixelReporter struct {
url string
}

func NewPixelReporter() *PixelReporter {
func NewPixelReporter(url string) *PixelReporter {
var pixelUrl string

// Attempt to get the value for the pixel URL from the environment. Fall back to default if that fails
if pixelUrl = os.Getenv(constants.AnalyticsPixelOverrideEnv); pixelUrl == "" {
// If a host is provided, use it. Otherwise, use the environment variable.
// Fall back to default if that fails.
if url != "" {
pixelUrl = url
} else if pixelUrl = os.Getenv(constants.AnalyticsPixelOverrideEnv); pixelUrl != "" {
pixelUrl = constants.DefaultAnalyticsPixel
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you make this a switch case? It's a bit frustrating to grok as is.

Copy link
Member Author

Choose a reason for hiding this comment

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

No longer needed with the new approach.


return &PixelReporter{pixelUrl}
}

Expand Down
8 changes: 5 additions & 3 deletions internal/analytics/client/sync/reporters/test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (

type TestReporter struct {
path string
host string
}

const TestReportFilename = "analytics.log"
Expand All @@ -23,8 +24,8 @@ func TestReportFilepath() string {
return filepath.Join(appdata, TestReportFilename)
}

func NewTestReporter(path string) *TestReporter {
return &TestReporter{path}
func NewTestReporter(path, host string) *TestReporter {
return &TestReporter{path, host}
}

func (r *TestReporter) ID() string {
Expand All @@ -36,11 +37,12 @@ type TestLogEntry struct {
Action string
Source string
Label string
URL string
Dimensions *dimensions.Values
}

func (r *TestReporter) Event(category, action, source, label string, d *dimensions.Values) error {
b, err := json.Marshal(TestLogEntry{category, action, source, label, d})
b, err := json.Marshal(TestLogEntry{category, action, source, label, r.host, d})
if err != nil {
return errs.Wrap(err, "Could not marshal test log entry")
}
Expand Down
3 changes: 3 additions & 0 deletions internal/constants/constants.go
Original file line number Diff line number Diff line change
Expand Up @@ -409,6 +409,9 @@ const SecurityPromptConfig = "security.prompt.enabled"
// SecurityPromptLevelConfig is the config key used to determine the level of security prompts
const SecurityPromptLevelConfig = "security.prompt.level"

// AnalyticsPixelOverrideConfig is the config key used to override the analytics pixel url
const AnalyticsPixelOverrideConfig = "report.analytics.endpoint"

// SvcAppName is the name we give our state-svc application
const SvcAppName = "State Service"

Expand Down
36 changes: 36 additions & 0 deletions test/integration/analytics_int_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -644,6 +644,42 @@ func (suite *AnalyticsIntegrationTestSuite) TestCIAndInteractiveDimensions() {
}
}

func (suite *AnalyticsIntegrationTestSuite) TestAnalyticsPixelOverride() {
suite.OnlyRunForTags(tagsuite.Analytics)

ts := e2e.New(suite.T(), false)
defer ts.Close()

cp := ts.Spawn("config", "set", constants.AnalyticsPixelOverrideConfig, "https://example.com")
cp.Expect("Successfully set config key")
cp.ExpectExitCode(0)

// Stop the service so that the next command will pick up the new config values.
// We need to do this because the analytics reporting instance will have values set that will
// be used until a new instance is created.
cp = ts.SpawnCmd(ts.SvcExe, "stop")
cp.ExpectExitCode(0)
Copy link
Contributor

Choose a reason for hiding this comment

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

The ACs call for the svc to pick up the change, so this should not be part of the test.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I've found a better pattern for this and the service is not picking up these changes. May apply this to the other PR.


cp = ts.Spawn("--version")
cp.Expect("ActiveState CLI")
cp.ExpectExitCode(0)

suite.eventsfile = filepath.Join(ts.Dirs.Config, reporters.TestReportFilename)
events := parseAnalyticsEvents(suite, ts)
suite.Require().NotEmpty(events)

foundCount := 0
for _, e := range events {
if e.URL == "https://example.com" {
foundCount++
}
}

// Because the service has already reported some events with the default configuration values,
// we expect to find at least one event with the new configuration values after the service is restarted.
suite.Greater(foundCount, 1)
}

func TestAnalyticsIntegrationTestSuite(t *testing.T) {
suite.Run(t, new(AnalyticsIntegrationTestSuite))
}
Loading