-
Notifications
You must be signed in to change notification settings - Fork 845
Analytics: Configure per account #4459
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.
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
base: master
Are you sure you want to change the base?
Conversation
478fb7d to
6ae59c9
Compare
bb7db0c to
c47be0f
Compare
analytics/build/builder.go
Outdated
| ) | ||
|
|
||
| // builders returns mapping between analytics module name and its builder. | ||
| // Since we no longer use vendor directories, we hardcode "modules" instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete the second comment.
router/router.go
Outdated
| shutdown, fetcher, ampFetcher, accounts, categoriesFetcher, videoFetcher, storedRespFetcher := storedRequestsConf.NewStoredRequests(cfg, r.MetricsEngine, generalHttpClient, r.Router) | ||
|
|
||
| analyticsRunner := analyticsBuild.New(&cfg.Analytics) | ||
| analyticsRunner := analyticsBuild.New(map[string]interface{}{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter here needs to be &cfg.Analytics.
Makefile
Outdated
|
|
||
| # build-analytics generates analytics/builder.go file which provides a list of all available analytics modules | ||
| build-analytics: | ||
| go generate analytics/modules.go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be analytics/build/build.go.
analytics/modules/agma/module.go
Outdated
| Enabled bool `mapstructure:"enabled" json:"enabled"` | ||
| Endpoint config.AgmaAnalyticsHttpEndpoint `mapstructure:"endpoint" json:"endpoint"` | ||
| Buffers struct { | ||
| EventCount int `mapstructure:"eventCount" json:"eventCount"` | ||
| BufferSize string `mapstructure:"bufferSize" json:"bufferSize"` | ||
| Timeout string `mapstructure:"timeout" json:"timeout"` | ||
| } `mapstructure:"buffers" json:"buffers"` | ||
| Accounts []config.AgmaAnalyticsAccount `mapstructure:"accounts" json:"accounts"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove all of the mapstructure annotations. Make this same change for the other modules (pubstack and filelogger).
analytics/modules/agma/module.go
Outdated
|
|
||
| var c Config | ||
| if cfg != nil { | ||
| if err := mapstructure.Decode(cfg, &c); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change this to jsonutil.Unmarshal since the data is passed in as JSON (make the same change for the other modules).
| @@ -1,4 +1,4 @@ | |||
| package filesystem | |||
| package filelogger | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline. We will change the folder name, the package name and update the template to refer to this as file so it matches the config and does not become a breaking change.
| // Config is the minimal configuration for the file logger analytics module. | ||
| // Empty Filename means the module is disabled. | ||
| type Config struct { | ||
| Enabled bool `json:"enabled"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did enabled exist before for file logger? If not, we should remove it and just rely on the presence of a filename in the config indicating that we want to enable this module.
| CurrencyConverter CurrencyConverter `mapstructure:"currency_converter"` | ||
| DefReqConfig DefReqConfig `mapstructure:"default_request"` | ||
| MaxRequestSize int64 `mapstructure:"max_request_size"` | ||
| Analytics map[string]interface{} `mapstructure:"analytics"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of the agma, pubstack and filelogger configs belong in their corresponding packages now because this is now a generic map for analytics module configurations.
config/config_test.go
Outdated
| agmaMap, _ := cfg.Analytics["agma"].(map[string]interface{}) | ||
| endpointMap, _ := agmaMap["endpoint"].(map[string]interface{}) | ||
| buffersMap, _ := agmaMap["buffers"].(map[string]interface{}) | ||
| cmpBools(t, "analytics.agma.enabled", false, agmaMap["enabled"].(bool)) | ||
| cmpStrings(t, "analytics.agma.endpoint.timeout", "2s", endpointMap["timeout"].(string)) | ||
| cmpBools(t, "analytics.agma.endpoint.gzip", false, endpointMap["gzip"].(bool)) | ||
| cmpStrings(t, "analytics.agma.endppoint.url", "https://go.pbs.agma-analytics.de/v1/prebid-server", endpointMap["url"].(string)) | ||
| cmpStrings(t, "analytics.agma.buffers.size", "2MB", buffersMap["size"].(string)) | ||
| cmpInts(t, "analytics.agma.buffers.count", 100, buffersMap["count"].(int)) | ||
| cmpStrings(t, "analytics.agma.buffers.timeout", "15m", buffersMap["timeout"].(string)) | ||
| accountsSlice, _ := agmaMap["accounts"].([]interface{}) | ||
| cmpInts(t, "analytics.agma.accounts", 0, len(accountsSlice)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove this and any other config checks for filelogger and pubstack since they are now the concern on the individual module packages due to the type change in config/config.go.
| shutdown, fetcher, ampFetcher, accounts, categoriesFetcher, videoFetcher, storedRespFetcher := storedRequestsConf.NewStoredRequests(cfg, r.MetricsEngine, generalHttpClient, r.Router) | ||
|
|
||
| analyticsRunner := analyticsBuild.New(&cfg.Analytics) | ||
| analyticsRunner := analyticsBuild.New(cfg.Analytics) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this no longer a pointer? cfg.Analytics could be nil if there aren't any analytics modules enabled.
Discussed offline. This should be ok as is.
analytics/modules/agma/module.go
Outdated
| return nil, nil | ||
| } | ||
|
|
||
| full := config.AgmaAnalytics{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All config structs for each analytics adapter should be defined in their corresponding module.go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add analytics/modules/agma/module_test.go
See modules/fiftyonedegrees/devicedetection/module_test.go as an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add analytics/modules/file/module_test.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add analytics/modules/pubstack/module_test.go
Implements the first part of #3809