uber-go / fx

A dependency injection based application framework for Go.
https://uber-go.github.io/fx/
MIT License
5.72k stars 289 forks source link

Breaking the glass on the stack #531

Closed glibsm closed 7 years ago

glibsm commented 7 years ago

Given a stack var Stack = fx.Options(A.Module, B.Module, C.Module)

What options are available to the user to either mutate or provide a custom A.Module. Right now the only option is to copy-pasta the stack (and therefore forego and branch the tight default version).

I'd like to think through being able to override one of the options, or ability to decorate the object right after it's creation before it's piped through the rest of the graph.

Option A: Do nothing

Declare this as SOL situation and have the service owner re-assemble the stack from the components.

This has two downsides that jump out:

Option B: Override approach

It could look something like:

import "default/stack"
import "fx"

func main() {
  // MyA.Module and A.Module provide the same types
  fx.New(stack.Stack, MyA.Module).Run() 
}

And would require Fx to understand which option is providing which types.

Option C: Decoration

This could be a new option that functions very similar to Invoke, however to solve the siblings problem it gets executed first before the rest of the constructors.

import "default/stack"
import "fx"

func main() {
  // <- A.Type is what's provided by A.Module
  fx.New(stack.Stack, fx.Decorate(func(a A.Type) { 
    a.mutate = true // ...
  }.Run()
glibsm commented 7 years ago

cc @bayesianmind

bayesianmind commented 7 years ago

Yes Option C is what I was thinking. The great thing about that approach is it lets users customize the stack in a few different ways without losing versioning and auto-updating of the original stack like you said. Specifically, users can trivially:

prashantv commented 7 years ago

Please don't do B -- if the same type is provided twice, it should be an error.

This doesn't strike me as a problem that needs to be solved right now, without waiting for more experience with how stacks are used. A copy+paste solution of stacks is not so bad initially while we get feedback on how it works for real users.

HelloGrayson commented 7 years ago

I have to agree with Prashant here.

I 100% don't want to back away from erroring on providing the same type twice.

Let's wait and see if this is absolutely needed before we consider adding it.

HelloGrayson commented 7 years ago

I suspect A might be just fine, since a stack can actually be comprised of stacks.

In this way, we can have a series of stacks that group modules that would likely go together.

HelloGrayson commented 7 years ago

Re Option C - I'm a bit nervous that this will make understanding what's in the object graph a not so simple task, which will degrade Fx's simplicity and confuse developers.

Let's not make a move unless A is really smelly.

bayesianmind commented 7 years ago

So my goal is to be able to wrap or mutate a type like zap.Logger. Is this the correct list of steps you need to do without an ability to decorate?

This seems terrible but I might be totally missing an obvious way to do it.

Option C would instead just be that you pass an extra option to the stack constructor: zapWrapper := func(log zap.Logger) zap.Logger { return wrap(log) } fx.New(fx.Stack(), fx.Override(zapWrapper))

HelloGrayson commented 7 years ago

I think what you'd want to do is:

  1. Don't provide zapfx since zap.Logger can only be provided once
  2. Provide your own module instead, foofx, which will be responsible for providing the zap.Logger
  3. Make foofx get the zap.Logger by calling into the zapfx module directly.
  4. Make modifications to the zap.Logger, and then provide it into the container

The YARPC Frontcar needs to do something very similar, and the above is the how we intend to deal with that.

bayesianmind commented 7 years ago

Sure that works, but what if zapfx actually needs a whole bunch of stuff out of the stack (complex dependency closure)? Then you're basically either sticking a 'private stack' inside of foofx that helps to build the complicated closure of deps or you do the same by hand. The way you mention will work so this isn't blocking but it still feels pretty clunky.

HelloGrayson commented 7 years ago

The other option is to add an invoke function which mutates the zap.Logger from your stack - it is a pointer after all.

HelloGrayson commented 7 years ago

@shah and @abhinav - how do you feel about C? To me, it's no different than the comment I just posted, so I'm not sure I'd want it as a first-class feature.

bayesianmind commented 7 years ago

Glib thought of that but he was pointing out you can't guarantee your invoke runs before any other, so it is non-deterministic. Even if that was solved somehow, that still only lets you mutate the state, not wrap it.

HelloGrayson commented 7 years ago

Can you describe what you mean by wrapping? What exactly do you need to do to the zap.Logger?

HelloGrayson commented 7 years ago

Another option might be to expand the zapfx module to allow for the functionality you need, depending on some other type already in the container.

It's hard to make a solid recommendation without knowing what you're after though.

bayesianmind commented 7 years ago

Your point about making tracing the stack components provenance more complicated is well taken, I don't know what you all had in mind for that. One thought I had was that we could record runtime.Caller() when you provide a type (or override it if we go with option C here). Users could ask dig to tell them the list of all the places that type was injected (in order, top one is the one that 'wins')

What I mean by wrapping is if I want to extend a type in AOP style. One scenario I had in mind that I'm pretty excited about is a tool kinda like mockgen that generates a wrapper for an interface. Imagine you want to time every invocation to a mysql library you got from the stack, you could just generate a timer wrapper and then decorate your mysql client from the stack with it. Bam, instant metrics.

bayesianmind commented 7 years ago

A more pressing usecase is for Catalyst users. config.Providers have a nested structure. We give Catalyst users by default a Catalyst config.Provider but if they want to extend it with additional sources of config (something like flipr, or their own config logic), then wrapping makes sense.

HelloGrayson commented 7 years ago

That would be pretty smooth - let's do this:

Can you schedule a jam sesh with Akshay, myself, and Glib? Maybe an hour so we can get into the deets?

bayesianmind commented 7 years ago

Sounds good! Will do

akshayjshah commented 7 years ago

Let's discuss in person.

(B) sounds particularly unappealing to me, for all the reasons already laid out. (C) just pushes the problem up a level - if there are two decorators for zap.Logger, which takes precedence?

glibsm commented 7 years ago

We're going with Option A from the fx perspective - do nothing.

In order to facilitate advances use cases, the modules will not only expose the top level fx.Option, but also the underlying constructors that will make breaking the glass and wrapping easier.