func (cv Value) PopulateStruct(target interface{}) error {
	if !cv.HasValue() {
		return nil
	}

	_, err := cv.valueStruct(cv.key, target)

	return err
}

right now when Value has no value, it is a noop. I think we should populate value in default tag instead.

This guy should probably have a map or something to memoize search results.

https://github.com/uber-go/fx/blob/master/config/provider.go#L58

Might need to also watch change notifications from any memoize'd keys or parents.

Having an http module in UberFX is confusing when YARPC also supports http.

Our customers will likely think "When do I use one over the other?". The answer is that all RPC traffic will go over YARPC, while browser applications will use the uhttp module. We can make this a bit more obvious by renaming the uhttp module.

My suggestion is web, but I think that browser could work as well. Something that makes it obvious that the intention of the module is to build browser applications.

Right now, modules/rpc is basically modules/yarpcthrift, ie it's not really for a generic rpc module, it's specifically for yarpc using the thrift encoding. Of course, you could add other rpc libraries (grpc for example) and other encodings (protobuf or avro for example) in the same package, but then you're talking about a lot of dependencies that aren't used for anyone importing this package.

I'd like to see something like:

modules/yarpc
modules/yarpc/thrift
modules/yarpc/protobuf
modules/grpc
modules/grpc/protobuf

for https://github.com/uber-go/fx/blob/master/ulog/log.go#L24

package-names: "Good package names are short and clear. They are lower case, with no under_scores or mixedCaps."

I like this convention because I know package names are all lower case so I can seperate them from private struct defined somewhere else. I think this convention applies to rename package as well.

So maybe 'stdliberr' or 'stderr'?

Before going 1.0, YARPC renamed the terms interceptor and filter to:

inbound middleware
outbound middleware

Why? We found that in introducing the interceptor/filter concept, that we were having to say "inbound/outbound middleware" anyways. This is because, without doing that, people were confused and didn't understand what these things did right away.

Of course, inbound and outbound middleware is much more verbose, which is why we started out with interceptor/filter in the first place, and because these are relatively common terms. In the end, we are happy we made the tradeoff, because the verbosity only occurs while setting up the framework, and not during request time.

The result has been great - users "get it" right away because the term is self describing and well understood. No need to define the term every time it's being used.

What do you say about using these terms in UberFx as well, so that both projects share terminology? I imagine this can only help our customers, where as having separate terms would only conflate the space.

cc @abhinav @kriskowal @willhug @bombela @blampe @prashantv @akshayjshah

Just want open a discussion about:

 // Handler is a context-aware extension of http.Handler.
type Handler interface {
	ServeHTTP(ctx context.Context, w http.ResponseWriter, r *http.Request)
}

vs

type Handler interface {
        ServeHTTP(ResponseWriter, *Request)
}

I am playing with std http with context recently and I found myself adopt easily. Here are reasons why I think following std defined interface is better:

1. context in one place. you always get context out of a request every time you use it and put context back every time you update it. if you look at uhttp/filters.go,

func (f contextFilter) Apply(ctx context.Context, w http.ResponseWriter, r *http.Request, next Handler) {
	ctx = fx.NewContext(ctx, f.host)
	next.ServeHTTP(ctx, w, r)
}

contextFilter failed to set context in request.
and at https://github.com/uber-go/fx/blob/master/modules/uhttp/filters.go#L80, tracingFilter overwrite request context directly instead wrapping original request context.
Since context is already in request, there is no need to pass context again and have the risk to make these two context out of sync.
Since request has Context and WithContext, user and std will use context in request. If we need to pick one source of truth, I think we should go with context in request.

2. no std http.Handler wrapper. 100% compatible with any http related package out there including std right away and always. I think this is important as well. When I select package, I will try to use ones follow std since std API appear to be stable and rarely change.

3. no "golang.org/x/net/context" incompatible issue. a handler implemented: ServeHTTP(ctx context.Context, w http.ResponseWriter, r *http.Request) can not be used as a

type Handler interface {
		ServeHTTP(ctx golang.org/x/net/context.Context, w http.ResponseWriter, r *http.Request)
}

context.Context can be passed as golnag.org/x/next/context.Context, but these two handler interface are not compatible. go1.8 has go fix which aiming to "solve" this issue, but following std interface will let us move a way from this mess.

Those are why I think we should use std http handler interface. Let's discuss :)

Currently ulog.Log interface is written with methods like this:

Debug(string, ...interface{})

• Is the first parameter a format string (like in Printf) or a plain message string?
• Are the arguments interpreted as in Printf or as alternating key-value pairs as in go-kit?

It's kind of an awkward name a little? Also is there duplication in the Reason and Error fields? If not, it would be nice to have some documentation. Note that in the Observer interface, you have OnShutdown(reason Exit) which is already confusing.

Following up our discussion yesterday: I would like to propose the following:

• fx.Context is a struct that embeds context.Context rather than an
  interface.
• UberFx values stored on fx.Context actually just use
  context.WithValue rather than wrapping a context.Context in a new
  type. Value accessors will be defined for all UberFx values.
• Corresponding methods will be declared on fx.Context which just call the
  accessor on itself.

Specifically,

package fx

type Context struct{ context.Context }

func (ctx Context) Logger() Logger {
    return GetLogger(ctx)
}

func GetLogger(ctx context.Context) Logger {
    // ..
}

This has a few advantages:

• Discoverability by having methods on fx.Context.

• Full compatibility with context.Context and WithValue. You won't lose
  UberFx information if you do,

  fxctx := fx.NewContext(ctx, host)
  ctx := fxctx.WithValue(...)

• Downgrading to context.Context and upgrading back to fx.Context just
  works.

  fxctx := fx.NewContext(ctx, host)
  var ctx2 context.Context = fxctx
  fxctx2 := fx.Context{ctx2}
  fmt.Println(fxctx.Logger() == fxctx2.Logger())  // true

The only thing to keep in mind is that accessors have to have reasonable
behavior for the case where a context.Context does not have UberFx
information on it.

Thoughts?

CC @breerly @kriskowal @willhug @sectioneight

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.

This is more of a mental note for myself, need to look into and document how dig behaves when encountering private types from other packages.

https://github.com/facebook/mention-bot

We've used this successfully to ensure the right people are looking at PRs and I think it would help a lot with cross-team collaboration.

@breerly @sectioneight thoughts?

Right now it's required that constructors return one and only one value: a pointer.

@dmcaulay pointed out it might be useful to return *ptr, error, in case constructors fail to initialize for one reason or another.

Hi all,

We have in the Host interface:

• metrics
• config
• trace
  ... more

Why not logging?

Took me 2 hours to realize that the service.WithModule().Build() initialize the zap global logger for us.

Now I do:

	// Initialize the service
	svc, err := service.WithModule(
		yarpc.New(yarpc.ServiceCreateFunc(server.NewYARPCThriftHandler)),
	).Build()
	if err != nil {
		panic(err)
	}

	log := zap.S()

	log.Info("Loading providers ", app.Config.Providers)
	log.Info("Starting the service ", svc.Name())
	svc.Start()

Why not expose zap.L() and zap.S() in the svc instance?

In this way I can "kind of" use svc as only dependency.

The term "graph" collides with "application graph" and "object graph", which is a term that relates to the users set of shapes that must come together.

Using "graph" in DIG's terminology is problematic for the same reason that using the term "inject" was: these terms exist to describe user-land concepts.

Overloading these terms creates confusion. Consider the following phrase:

The proposal is that all deps in the application graph, including clients and handlers, get registered into dig's graph so that the full graph can be resolved.

Here I used the term "application graph" to mean "all the shapes in user-land that must be wired using DI". This is confusing because DIG has a term "graph", which overloads the graph with the things which must make their way into the graph.

My suggestion is to use the classical term for this: "Container".

All deps in the application graph, including clients and handlers, get registered into a DIG container so that the graph can be resolved.

This will prevent confusion and make it easier for users to understand and utilize the concepts in DIG:

// create a new dependency injection container
container := dig.NewContainer()

// register all shapes in the application graph
container.Register(...)

// resolve the application graph
container.Resolve()

While trying to load an integer string to a float64 configuration value, config.Value.PopulateStruct() simply panic:

panic: interface conversion: interface is int, not float64
goroutine 1 [running]:
panic(0x984040, 0xc420231ec0)
  /usr/local/Cellar/go/1.7.5/libexec/src/runtime/panic.go:500
go.uber.org/fx/config.convertValueFromStruct(...)
  go.uber.org/fx/config/value.go:352
go.uber.org/fx/config.Value.valueStruct(...)
  go.uber.org/fx/config/value.go:513
go.uber.org/fx/config.Value.PopulateStruct(...)
  go.uber.org/fx/config/value.go:400
go.uber.org/fx/config.Value.valueStruct(...)
  go.uber.org/fx/config/value.go:524
go.uber.org/fx/config.Value.PopulateStruct(...)
  go.uber.org/fx/config/value.go:400
go.uber.org/fx/config.Value.valueStruct(...)
  go.uber.org/fx/config/value.go:524
go.uber.org/fx/config.Value.PopulateStruct(...)
  go.uber.org/fx/config/value.go:400

A value in the form of an integer, e.g. "1" should be seamlessly converted to a float value with no issue.

Also, it took me quite a while to figure out the actual offending key. It would have saved me the time if key name is part of the failure message.

To reproduce the problem, simply create a config struct containing a float64 member, then call Value.PopulateStruct() to feed that value with any config provider reading data from an integer string.

Need to take a pass. Includes things like:

err := dig.Resolve(&o) // notice the pointer to a pointer as param type
if err == nil {
    // o is ready to use
}

err = g.Resolve(&f1)
require.NoError(t, err)

Understand that the common way to print the actual value of a "Value" is to call Value.Value(), however, printing a raw object with "%v" causes stack overflow is something worth notice.

root := config.Load()
cfg := root.Get("test_sec")
fmt.Printf("%v", cfg)

ends up with:

runtime: goroutine stack exceeds 1000000000-byte limit
fatal error: stack overflow

For yaml files, it feels more natural to have:

logging:
  text_formatter: true

Instead of:

logging:
  textFormatter: true

With how the config reads yaml files right now, the latter will work, while the former will not, unless you have explicit yaml tags (right?). Could we make this magical to do either/or? I haven't looked into it at all, so it may take a change to the yaml library.

Also something to note that you might want to use: https://github.com/peter-edge/pkg-go/blob/master/yaml/pkgyaml.go#L55

What this allows me to do is to have either yaml or json files, and do the same thing. This also makes parsing work if there are only json tags without yaml tags, which gets REALLY useful when you have protobuf/thrift entities like

message Config {
  bool stdout = 1;
}

And you can just have protobuf/thrift generate the structs, which (at least for protobuf) have json tags already, and then read them in using that function.

I'd suggest to add the f variants of the logger.

log.Infof("hello %v", world)

or find a way to make this possible in k:v

log.Infof("debug", "world %+v", foo, "bar %q", bar) 

Disclaimer: These are opinions based on a superficial overview of the UberFx source code.

I have two concerns when it comes to UberFx code complexity.

1. Startup doesn't need to be asynchronous. There is a lot of code for synchronizing multithreaded startup. This seems unnecessary for a couple reasons:

   • Startup isn't that slow.
   • If startup for a module is too slow, then you can distribute using roles.

2. Modules shouldn't worry about managing their backend. This is not natural and it's difficult to test. I'd prefer to formalize backends and make initialization more explicit. Here's some pseudocode illustrating this startup flow:

h := host.New(...)

// initialize observer with host
o := newObserver(h)

// add modules synchronously
backends := map of backends
for _, newModule := range modules {
  m := newModule(h)
  b, ok := backends[m.Backend()]
  if !ok {
    // get backend constructor from map of registered constructors
    newBackend, ok := backendConstructors[m.Backend()]
    if !ok {
      return nil, errors.New("unknown backend")
    }
    var err error
    if b, err = newBackend(h); err != nil {
      return nil, err
    }
    backends[m.Backend()] = b
  }
  err = b.AddModule(m)
  // handle error
}

// when we start we start backends
for _, b := range backends {
  err := b.Start()
  // handle error
}

Let's take a look at what the YARPC backend might do:

func New(h service.Host) *backend {
  // adjust configuration
  // initialize dispatcher
}

func (b *backend) AddModule(m service.Module) error {
  // maybe type assert to thrift module so it get access the thrift service
  // register service handler
}

func (b *backend) Start() error {
  // start dispatcher
}

This way we don't have to check if the dispatcher has already been initialized, started, stopped in the module functions. We can eliminate all that logic completely and rely on UberFx to startup backends responsibly. The module code also becomes simpler, because all it needs to do is create the thrift service.

I may be trivializing things, but I think separating concerns when it comes to backends and modules could help simplify other modules as well.

All dig calls return an error, but it's possible that some reflect calls might fail. Should defer a catch

I know everyone likes their code coverage, richgo, etc heh, but consider the following:

$ time go test $(go list ./... | grep -v vendor)
...
real	0m5.135s
user	0m21.234s
sys	0m4.598s

$ time make test
..
real	0m58.386s
user	5m24.928s
sys	1m10.965s

I know that the make target is trying to do some smart things, but when I'm developing, most of the time I just want to know if the code builds and the tests pass. I had to turn off neomake (the vim plugin I have setup to run make on every save of a go file) because my computer slowed way down and my computer's fan wouldn't stop running. I actually almost max out every processor.

Can we do something about this for day-to-day development? I also wish I could just specify specific packages with whatever is eventually done.

Currently, there is no way to inject anything other than constructors into fx.New.

This is a really nice UEX, until we need to inject something. For example, we need to be able to inject a logger. In the future, I could see us needing even more.

We should make this API future-proof. Here's a strawman to get the convo going:

app := fx.New(
  fx.WithLogger(logger),
)
app.Provide(const1, const2, const3)
app.RunForever()

Thoughts?

For Job https://travis-ci.org/uber-go/fx/jobs/207786565, logs:

2017/03/04 23:13:11 PASS | HTTPModule_StartsAndStops (0.00s)
2017/03/04 23:13:11 START| BuiltinHealth_OK
2017/03/04 23:13:11 | exit status 2
2017/03/04 23:13:11 FAIL | go.uber.org/fx/modules/uhttp 0.241s
2017/03/04 23:13:11 | {"level":"error","ts":1488669191.812442,"caller":"/tmp/go-build215164269/go.uber.org/fx/service/_test/_obj_test/manager.go:276","msg":"Failure to close tracer","error":"Failed to close","errorVerbose":"Failed to close\ngo.uber.org/fx/testutils.ErrorCloser.Close\n\tgo.uber.org/fx/testutils/metrics.go:40\ngo.uber.org/fx/testutils.(*ErrorCloser).Close\n\t:2\ngo.uber.org/fx/service.(*manager).shutdown\n\t/tmp/go-build215164269/go.uber.org/fx/service/_test/_obj_test/manager.go:274\ngo.uber.org/fx/service.checkShutdown\n\tgo.uber.org/fx/service/manager_test.go:214\ngo.uber.org/fx/service.TestManagerShutdown_TracerCloserError\n\tgo.uber.org/fx/service/manager_test.go:209\ntesting.tRunner\n\t/home/travis/.gimme/versions/go1.8.linux.amd64/src/testing/testing.go:657\nruntime.goexit\n\t/home/travis/.gimme/versions/go1.8.linux.amd64/src/runtime/asm_amd64.s:2197","stacktrace":"goroutine 57 [running]:\ngo.uber.org/fx/vendor/go.uber.org/zap.takeStacktrace(0xc420428800, 0x800, 0x800, 0xd98400, 0x48, 0x0)\n\tgo.uber.org/fx/vendor/go.uber.org/zap/stacktrace.go:39 +0xe2\ngo.uber.org/fx/vendor/go.uber.org/zap.Stack(0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0)\n\tgo.uber.org/fx/vendor/go.uber.org/zap/field.go:205 +0x9c\ngo.uber.org/fx/vendor/go.uber.org/zap.(*Logger).check(0xc420538660, 0x2, 0xad6785, 0x17, 0xc420434900)\n\tgo.uber.org/fx/vendor/go.uber.org/zap/logger.go:268 +0x300\ngo.uber.org/fx/vendor/go.uber.org/zap.(*Logger).Error(0xc420538660, 0xad6785, 0x17, 0xc420434900, 0x1, 0x1)\n\tgo.uber.org/fx/vendor/go.uber.org/zap/logger.go:176 +0x56\ngo.uber.org/fx/service.(*manager).shutdown(0xc4200a6c80, 0x0, 0x0, 0xacf1f5, 0x7, 0xc420605ec0, 0xdffb00, 0x0, 0x0)\n\t/tmp/go-build215164269/go.uber.org/fx/service/_test/_obj_test/manager.go:276 +0xc68\ngo.uber.org/fx/service.checkShutdown(0xc420517ba0, 0xc4200a6c80, 0xda8e01)\n\tgo.uber.org/fx/service/manager_test.go:214 +0x77\ngo.uber.org/fx/service.TestManagerShutdown_TracerCloserError(0xc420517ba0)\n\tgo.uber.org/fx/service/manager_test.go:209 +0x1af\ntesting.tRunner(0xc420517ba0, 0xb37ac0)\n\t/home/travis/.gimme/versions/go1.8.linux.amd64/src/testing/testing.go:657 +0x108\ncreated by testing.(*T).Run\n\t/home/travis/.gimme/versions/go1.8.linux.amd64/src/testing/testing.go:697 +0x544\n"}
2017/03/04 23:13:11 PASS | ManagerShutdown_TracerCloserError (0.00s)

Creating here, because I am working from my personal laptop.

How do you like using uhttp, ulog, uauth?
Alternate package naming - fxhttp, fxlog, fxauth, etc

u-stuff is derived from uberfx. Packages in Fx can be super generic to use, but often clash with golang's package names. The IDE gets confused by which package to use, and developers end up with mixed bags of object initialization.

While trying to load an integer string to a float64 configuration value, config.Value.PopulateStruct() simply panic:

panic: interface conversion: interface is int, not float64
goroutine 1 [running]:
panic(0x984040, 0xc420231ec0)
  /usr/local/Cellar/go/1.7.5/libexec/src/runtime/panic.go:500 +0x1a1
go.uber.org/fx/config.convertValueFromStruct(...)
  go.uber.org/fx/config/value.go:352 +0x3e0
go.uber.org/fx/config.Value.valueStruct(...)
  go.uber.org/fx/config/value.go:513 +0x63f
go.uber.org/fx/config.Value.PopulateStruct(...)
  go.uber.org/fx/config/value.go:400 +0xf0
go.uber.org/fx/config.Value.valueStruct(...)
  go.uber.org/fx/config/value.go:524 +0x1fba
go.uber.org/fx/config.Value.PopulateStruct(...)
  go.uber.org/fx/config/value.go:400 +0xf0
go.uber.org/fx/config.Value.valueStruct(...)
  go.uber.org/fx/config/value.go:524 +0x1fba
go.uber.org/fx/config.Value.PopulateStruct(...)
  go.uber.org/fx/config/value.go:400 +0xf0

A value in the form of an integer, e.g. "1" should be seamlessly converted to a float value with no issue.

Also, it took me quite a while to figure out the actual offending key. It would have saved me the time if key name is part of the failure message.

To reproduce the problem, simply create a config struct containing a float64 member, then call Value.PopulateStruct() to feed that value with any config provider reading data from an integer string.

We should move dig to its own repo (go.uber.org/dig perhaps). This is probably
already on your roadmap but I wanted to explicitly create an issue to track
this.

With the current implementation we can get runtime errors ... for logging. Even if more verbose I'd follow the withFields thing we used before.

make lint test calls into the makefile in examples/keyvalue, which does it's own dependency management. It would be nice if this was somehow merged, so that glide isn't called twice. I'd honestly just put the example makefile goals into the main makefile, including any extra dependencies. This makes make lint test much slower.

I don't see any docs or tests that describe how deep merge semantics work - are maps and objects merged, or is this just top-level?

https://github.com/uber-go/fx/tree/master/config

refer to this: https://github.com/uber-go/fx/blob/master/modules/uhttp/health.go#L32

I like the idea behind our internal gocommon health package. When we used it, we made some twists.

1. instead of a Check func(), we defined an interface:

// Checkable is anything implemented Health method.
type Checkable interface {
	// Health takes a context and return error if health check failed.
	Health(ctx context.Context) error
}
// also func
type CheckableFunc func(ctx context.Context) error
func (f CheckableFunc) Health(ctx context.Context) error { return f(ctx) }

so all gateway or storage interfaces can be health checked once they implemented this interface. We can even automatically wrap the dependency's meta.thrift to it.

2. we also add a Checker struct can do:

{
    // Add adds a Checkable map to a given name to Checker as a hard dependency.
    Add(name string, check Checkable)
    
    // AddSoft adds a Checkable map to a given name to Checker as a soft dependency.
    // If the check on soft dependency fails, checker will generate a warning message without make health check fail.
    AddSoft(name string, check Checkable)

    // ThriftHealthFunc returns a thrift health check function.
    ThriftHealthFunc() thrift.HealthFunc

    // HTTPHealthFunc returns a http health check function.
    HTTPHealthFunc(ctx context.Context) http.HandlerFunc
}

I think this is quite flexible compare to one global health checker since nowadays a service can serving more than one "service" with both soft and hard dependencies.
Also http.HandlerFunc can be mount under any route and thrift.HealthFunc can be mounted to tchannel.Service served on this server with Server.RegisterHealthHandler easily.
One issue is YARPC doesn't have Server.RegisterHealthHandler right now. I opened a ticket to track this: yarpc/yarpc-go#733.

3. The remaining are the same. host name, timestamp, pid, response time, status will all be returned in health report.

What's you guys' thought on these interface/struct? What other features or integrations you think should be included in health check package?

Most handlers require access to the host to get to metrics, logging and config.

On successful uberfx service start, we should inject that into the default graph so writing handler constructors becomes a lot easier.

Open to discussion if we don't think that's a good thing to do.

Opening an issue for discussion.
GFM-198

As methods are currently described and implemented as methods that would return the value in the desired type, and panic when they cannot convert. For example, AsString() method on ConfigValue currently returns fmt.Sprintf("%v", value). As a result AsString() returns string representation of an object, including slice or map.

Is this correct behavior for the API? Should there be a stricter API which would error out on any type mismatch?

Alternate behavior is that we want error to be returned if there is a type mismatch rather than casting value into the desired type. We can have two options - update As* methods with much stricter type checks and errors, or provide some form of strict APIs StrictString() or just String() APIs?

Although I might be minority here. To me throwing an error seems overkill considering the caller specifically requested ConfigValue as described in the method signature. The caller shouldn't be expecting error on mismatched type and avoid getting into expected panicking code paths.

Thoughts?

At the moment, it's possible to Register a cyclical graph.
This should be defected, perhaps at registration time.

Instead of only having default deadlines created when calling RunForever, I'd like to:

1. Make context.Context optional when calling Start/Stop by creating an fx.WithContext option
2. When the fx.WithContext option is not provided, Start/Stop will create their own context with default deadlines.

This would effectively make the common case:

app := fx.New()
app.Start()
app.Stop()

Where the context could be specified manually when needed:

app := fx.New()
app.Start(fx.WithContext(context.WithTimeout(context.Background(), time.Second*5)))
app.Stop(fx.WithContext(context.WithTimeout(context.Background(), time.Second*10)))

This balances the API because:

• Makes Start/Stop less reliant on being called by RunForever, since they effectively set their own deadlines.
• Keeps RunForever as a convenience, but doesn't force Start/Stop to be difficult just so RunForever can seem convenient

ModuleCreateFunc is defined as,

type ModuleCreateFunc func(ModuleCreateInfo) ([]Module, error)

This makes it ugly to have types which act as module builders. Imagine,

service.WithModules(
    mymodule.Module{
        Foo: bar,
    },
)

You can't do that because WithModule accepts Funcs only. You'd instead have to
do the following, which is a poor API IMO.

service.WithModules(
    mymodule.Module{
        Foo: bar,
    }.Create,
)

I propose adding a ModuleCreator interface. ModuleCreateFunc (possibly
renamed to ModuleCreatorFunc) then becomes one of the implementations of
that interface.

type ModuleCreator interface {
    func Create(ModuleCreateInfo) ([]Module, error)   
}

The documented way is:

  svc, err := service.WithModules(
    rpc.ThriftModule(
      rpc.CreateThriftServiceFunc(NewYarpcThriftHandler),
      modules.WithName("keyvalue"),
    ),
  ).Build()

Besides this, it support to use Builder:

builder := server.NewBuilder().WithModules(
    rpc.ThriftModule(
      rpc.CreateThriftServiceFunc(NewYarpcThriftHandler),
      modules.WithName("keyvalue"),
    ),
)
svc, err := builder.Build()

and using Owner:

owner, err := service.New()
err = owner.AddModules(
    rpc.ThriftModule(
      rpc.CreateThriftServiceFunc(NewYarpcThriftHandler),
      modules.WithName("keyvalue"),
    ),
)

All three ways look pretty similar, but all different.

I think a good practice is to provide one full-featured build path, and probably provide one shortcut for fast coding. Too many building ways make developers write divergent code and increase the difficulty to read across many repos. My proposal is to

1. remove Builder struct
2. rename service.New to server.NewOwner
3. add new shortcut function service.ServeModule(module ModuleCreateFunc, options ...Option) error

Any thoughts? I can send diff if this sounds good.

not sure if some pre defined lite weight rpc middlewares fall into fx bucket. But almost every service I create, I will need to have a middleware(wrapper) to log panic stack tracer to sentry and return 500 to client.

for example for http I have:

// WithRecovery returns a recovery middleware handle painc.
func WithRecovery() Middleware {
	return func(next http.Handler) http.HandlerFunc {
		return func(w http.ResponseWriter, r *http.Request) {
			defer func() {
				if r := recover(); r != nil {
					log.WithField("Error", string(debug.Stack()).Errorf("http handler panic: %v", r)
					http.Error(w, "server_error", http.StatusInternalServerError)
				}
			}()
			next.ServeHTTP(w, r)
		}
	}
}

for tchannel I have:

type recoverableTChanServer struct {
	thrift.TChanServer
}

func (tChanServ *recoverableTChanServer) Handle(ctx thrift.Context, method string, protocol athrift.TProtocol) (ok bool, resp athrift.TStruct, err error) {
	defer func() {
		if r := recover(); r != nil {
			log.WithField("Error", string(debug.Stack())).Errorf("tchannel handler panic: %v. ", r)
			ok = false
			resp = nil
			err = errors.New("server panic")
		}
	}()
	return tChanServ.TChanServer.Handle(ctx, method, protocol)
}

// NewRecoverableServer wraps an existing TChanServer to make it recoverable from panic.
func NewRecoverableServer(serv thrift.TChanServer) thrift.TChanServer {
	return &recoverableTChanServer{TChanServer: serv}
}

It could be nice if this can be included in service framework.

Might be nice. Note this will break like everyone's usage of FX right now, so not so nice. Point being, Start isn't really just starting it, I want it to be clear that it's blocking from the name. Serve might also work.

In practice I've found codecov.io to be a much better tool than coveralls:

• Able to run locally, and the coverage always matches what codecov.io says
• The coverage comments on PRs gamifies not lowering coverage in a way I've seen lead to better PRs
• The browser plugin shows inline lines that aren't covered, leading to better code reviews

I'd like to switch to this while the source code is still small and relatively simple.

I'm not sure if this should be filed here or JIRA, let me know.

CircleCI just released their beta recently which includes support for docker image caching. Given that we are moving to doing ci builds within a docker container, this should significantly speed up the build. I use CircleCI for all my personal projects and generally prefer it over Travis, I'd like to switch to it. This requires someone with admin permissions on uber-go to allow CircleCI.

https://circleci.com/beta-access
https://circleci.com/docs/2.0

@prashantv @sectioneight @glibsm @breerly

as long as they are used in public functions, they should be public IMO.

IIRC, yarpc is just one type of RPC system which could be supported as modules. Similarly, gRPC may be supported too in the future.

After the middleware renaming, I feel it is quite nature to move uhttp/client into uhttp.
some of my thoughts:

1. All uhttp APIs are in one package like std http and xhttp. it will be more easier to read through the APIs.

2. client is a package name you may want to avoid to use for public facing. Because it is a common package name and even common variable name. an example is our doc example:

   client := client.New(svc)
   // client package cannot be used in this scope anymore!

   It is also not specific about what client it is. So when our users use it, they probably gonna rename it to something like uhttpclient.

3. Filters have not naming overlap now. So it is more nature to have both InboundMiddleware and OutboundMiddleware live in the same package.

Overall, only two public APIs will be changed:
New -> NewClient and Executor -> OutboundExecutor

Let's discuss about this change. Maybe there are some good reasons to keep them in client package.

There are currently three usages of function Register which have entirely different applications:

1. dig.Register inserts into the dependency graph.
2. task.Register registers a function into the async task module, or an interface implementation.
3. dispatcher.Register registers []transport.Procedure for YARPC.

Two out of the three are under UberFx control, one comes from YARPC.

An earlier suggestion was to rename dig.Register to dig.Provide. I am now in favor of the change to try and break up the overloaded word.

My service exposes both authenticated and non-authenticated HTTP endpoints. The defaultInboundMiddlewareChainBuilder is automatically adding auth client from the service level and uauth appears to only allow a single auth client per service.

Is there a way for me to provide both authenticated and non-authenticated endpoints without checking for specific paths in a custom auth client?

Re: #267, the yarpc Module create function should not depend on a specific encoding @breerly

Looks like for a project that only has uberfx in its glide.yaml, that glide is not respecting fx's requirements in it's own glide.yaml.

From the current fx master e0c5d60f3dba78297ff20b61ba1626defb86efaf glide.yaml:

- package: go.uber.org/yarpc
  version: v1.0.0-rc1

And it's glide.lock:

- name: go.uber.org/yarpc
  version: 6048dff5656e15859918b9d40fdf12199d0c6e3d

My code's glide.yaml:

import:
- package: go.uber.org/fx
  version: master

And my glide.lock after running glide up:

- name: go.uber.org/yarpc
  version: 7c6a4c5618cb38228ef9f0eb85ea71d8fc5af2c4

That's an old version from halloween

commit 7c6a4c5618cb38228ef9f0eb85ea71d8fc5af2c4
Author: Grayson Koonce <[email protected]>
Date:   Mon Oct 31 17:08:57 2016 -0700

    Use standard library's context package (#394)