Allow configuration of analytics pixel URL#3718
Conversation
|
|
||
| // 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 | ||
| } |
There was a problem hiding this comment.
Could you make this a switch case? It's a bit frustrating to grok as is.
There was a problem hiding this comment.
No longer needed with the new approach.
| // 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) |
There was a problem hiding this comment.
The ACs call for the svc to pick up the change, so this should not be part of the test.
There was a problem hiding this comment.
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.
internal/analytics/analytics.go
Outdated
| } | ||
|
|
||
| func RegisterConfigListener(cfg *config.Instance) error { | ||
| configMediator.AddListener(constants.AnalyticsPixelOverrideConfig, func() { |
There was a problem hiding this comment.
Far as I can tell this does not speak with the state-svc. So a listener placed on the svc will not fire if the config was changed on the state tool side.
I like this pattern though, and it seems to me we ought to support this across state tool and the svc. It also means the code as is is completely self sufficient aside from the state-svc AC, but if we address that separately this code will automatically get the benefit.
I've filed this as a followup: https://activestatef.atlassian.net/browse/CP-1083, and we'll drop the AC from the current set of stories regarding the svc picking up the change. I don't think it's critical for the milestone we're targeting.
Good thinking on taking this approach!
There was a problem hiding this comment.
I'm fairly certain that the service will pick up this change. When we set a config value the runner makes a request to the service which in turn notifies the listeners in the service.
There was a problem hiding this comment.
The changes to the integration test should prove that the service will pick up the config changes.
There was a problem hiding this comment.
Ohh nice! I wasn't grokking that yesterday. Thanks for digging! 🙏
cmd/state-svc/main.go
Outdated
| if err := analytics.RegisterConfigListener(cfg); err != nil { | ||
| multilog.Critical("Could not register config listener: %v", errs.JoinMessage(err)) | ||
| exitCode = 1 | ||
| return | ||
| } |
There was a problem hiding this comment.
Analytics is already using a single instance that is propagated everywhere, so there should be no need for globals. You can do the config listener registration in analytics.New().
There was a problem hiding this comment.
I can't find a confirmation of this. There is no analytics.New(). We have multiple analytics clients and while the async client is created once and passed around throughout the state tool the sync client has multiple instantiations in the state service.
There was a problem hiding this comment.
Sorry, it's not analytics.New() it's sync|async.New(). eg. #3718 (comment)
I only see a single invocation of New() in any of our cmd's? Far as I can tell my point still stands.
There was a problem hiding this comment.
internal/analytics/analytics.go
Outdated
| Close() | ||
| } | ||
|
|
||
| var AnalyticsURL string |
There was a problem hiding this comment.
This should still be recorded on the PixelReporter. Unlike with the API host the use of analytics is not pervasive, it's all a single instance.
There was a problem hiding this comment.
I've moved this to the pixel reporter but as stated above I can't confirm that there will be a single instance
Naatan
left a comment
Naatan
left a comment
There was a problem hiding this comment.
I'm still maintaining there is no need for globals for this PR:
Pointed out the multiple sync clients here #3718 (comment) |
Naatan
left a comment
Naatan
left a comment
There was a problem hiding this comment.
Thanks for digging up the other reference, though this still doesn't change that we don't need globals here. Both instances are receiving the cfg and are able to record the stored host to their instance. There should be no need for a global.
| url string | ||
| cfg *config.Instance |
There was a problem hiding this comment.
I think we can still keep the url here, no? It's static, it only changes when the config listener fires, at which point we can update the value here. The config listener is set at construction time, so we can just loose the config after the listener is placed.
| var ( | ||
| pixelUrl string | ||
|
|
||
| envUrl = os.Getenv(constants.AnalyticsPixelOverrideEnv) | ||
| cfgUrl = r.cfg.GetString(constants.AnalyticsPixelOverrideConfig) | ||
| ) | ||
|
|
||
| switch { | ||
| case envUrl != "": | ||
| pixelUrl = envUrl | ||
| case cfgUrl != "": | ||
| pixelUrl = cfgUrl | ||
| default: | ||
| pixelUrl = constants.DefaultAnalyticsPixel | ||
| } | ||
|
|
||
| pixelURL, err := url.Parse(pixelUrl) |
There was a problem hiding this comment.
Suggest making this its own function that gets invoked both in the constructor and when the config value changes. You could make that a private method on PixelReporter which then also updates the url.
2 participants
No description provided.