Kill a lot of the globals

[bufdev]

I ran into a testing bug where I was scoping all metrics for a given module to host.Metrics().SubScope("modules").SubScope(moduleName), which I think is desired (if I have two http modules, I would prefer to have stats on them separately). I found out this breaks the flow completely - I think it would be nice to take out the globals in the internal/stats packages, and setup any middleware etc to use a wrapping interface that exposes the metrics we care about.

[bufdev]

I actually want to rename this to "let's try to get rid of as many globals as possible". Having gone through the code with the module rewrite PR, there's a lot of places where what I would expect to happen will not happen.

[bufdev]

I did a quick search, this is a rough list of the globals I think should be killed in theory:

modules/uhttp/internal/stats/metrics.go:38:2: HTTPTags
modules/uhttp/internal/stats/metrics.go:44:2: HTTPPanicCounter
modules/uhttp/internal/stats/metrics.go:46:2: HTTPAuthFailCounter
modules/uhttp/internal/stats/metrics.go:48:2: HTTPMethodTimer
modules/uhttp/internal/stats/metrics.go:50:2: HTTPStatusCountScope
metrics/metrics.go:35:2: _scopeFunc
metrics/metrics.go:36:2: _mu
metrics/metrics.go:37:2: _frozen
metrics/nop_reporter.go:52:5: NopCachedStatsReporter
metrics/nop_reporter.go:77:5: NopCachedCount
metrics/nop_reporter.go:86:5: NopCachedGauge
metrics/nop_reporter.go:95:5: NopCachedTimer
modules/rpc/thrift.go:33:5: _setupMu
modules/rpc/yarpc.go:55:2: _dispatcherMu
modules/rpc/yarpc.go:58:2: _dispatcherFn
modules/rpc/yarpc.go:61:2: _starterFn
modules/rpc/yarpc.go:69:2: _controller
ulog/log.go:90:2: _setupMu
ulog/log.go:91:2: _once
ulog/log.go:92:2: _std
modules/rpc/internal/stats/metrics.go:40:2: RPCTags
modules/rpc/internal/stats/metrics.go:46:2: RPCAuthFailCounter
modules/rpc/internal/stats/metrics.go:48:2: RPCHandleTimer
modules/rpc/internal/stats/metrics.go:50:2: RPCPanicCounter
service/service_setup.go:37:5: _simpleCtx
auth/uauth.go:32:2: _registerFunc
auth/uauth.go:33:2: _setupMu
modules/task/internal/stats/metrics.go:35:2: TaskTags
modules/task/internal/stats/metrics.go:40:2: TaskExecutionCount
modules/task/internal/stats/metrics.go:42:2: TaskPublishCount
modules/task/internal/stats/metrics.go:44:2: TaskExecuteFail
modules/task/internal/stats/metrics.go:46:2: TaskPublishFail
modules/task/internal/stats/metrics.go:48:2: TaskExecutionTime
modules/task/internal/stats/metrics.go:50:2: TaskPublishTime
config/config.go:53:2: _setupMux
dig/default.go:24:5: defaultGraph
modules/task/task.go:39:2: _globalBackendMu
modules/task/task.go:40:2: _globalBackend
modules/task/task.go:41:2: _asyncMod
modules/task/task.go:43:2: _once

[bufdev]

I think in general, I'd like to be able to have two independent service.Manager instances that have no effect on each other. I know the general thinking is "well most people won't want this", but 1. I think then we might as well just make the Manager concept a single global instance inside service 2. I can think of situations like integration testing (imagine two Managers controlling Modules that talk to each other) where this could be super useful

[glibsm]

I agree with the general sentiment of removing the globals, but sometimes they are necessary to provide hook points for private implementations. We use that in several places like, for instance, the metrics backend comes from a closed source repo.

[madhuravi]

+1 for Glib, there are useful in some cases to make code more easy and intuitive to use. We can go after specific globals that kill thread safety or make testability harder etc., not all of them.

[HelloGrayson]

provide hook points for private implementations.

@glibsm can you be more specific here?

AFAICT, using dependency injection to accomplish polymorphism (what I think you're talking about here) is more obvious for users and better for testing and concurrency. You no longer need to wonder what globals exist or what side-effects occur, since everything is explicit and defined at the func/struct level.

At face value, I agree with @peter-edge here - it would be great if the user experience for UberFX was:

1. Instantiate a few UberFX shapes
2. Combine those with the required shapes
3. That's all

In general, we've pushed really hard not to have any globals in YARPC, since they compose very poorly, make testing harder, and introduce problems with concurrency.

[ghost]

@breerly e.g. RPCInit Dispatcher is different from YARPC Dispatcher, this could go away with DI though.

[glibsm]

@breerly Take a look, for instance, at RegisterRootScope and how it's used in the private repo to hook up the reporter.

I agree that DI can probably help with this, or completely replace the practice alltogether, but we're nowhere near ready to start replacing these registration functions with dig, which is still in short trousers.

[HelloGrayson]

100% agreed - let's grow into it, with the goal of using DI and dig to accomplish this 🗡

[bufdev]

#316, #317, and #318 gets rid of the relevant global variables for statistics in modules.

[bufdev]

#323 gets rid of the relevant global variables in the metrics package.

[HelloGrayson]

Closing - the modular branch, which reflects the direction of this project, contains absolutely 0 globals.