Allow run a trace in a closure

[cschneider]

See issue #270

[codefromthecrypt]

I like the exploration, though not crazy about the impl, yet. I think looking at before/after in unit tests might make it more clear what we're gaining.

[codefromthecrypt]

this is java 8 (and we are java 6)... I think this import will break animal sniffer unless it is excluded

[codefromthecrypt]

if something throws an exception, you should probably propagate it vs wrap

[cschneider]

The problem is that Callable.call throws Exception. If I propagate this I force people to catch it. I can improve this though to not wrap RuntimeException.

[codefromthecrypt]

iirc Runnable doesn't have declared exceptions, so this will just wrap runtime exceptions in another runtime exception?

[cschneider]

Yes .. This is wrong. I will remove the catch.

[cschneider]

I pushed a new commit now that should fix the issues.

[codefromthecrypt]

cool. one thing I'm curious about is why we need to use Callable. If the goal of this is to support lambda, then exceptions need to be wrapped anyway, Callable wasn't designed for lambda. If we want lambda, I'd probably go for a custom func1 interface (which lambda can implement) Lambda could implement that with a j.u.Function, any functional interface or even a method reference. if we went this way, I'd just mention the rationale of the custom function type is the language level.

[cschneider]

I chose Callable and Runable as they are pre Java 8 interfaces. So people can already use closures if they run on Java8 but Brave itself does not have to depend on Java 8.
I think we could define a custom interface like Callable but without "throws Exception". If you prefer this then I can change the PR.
Typically I try to avoid creating new interfaces if there is an existing one as people then depend on something proprietary. In case of brave this should be no problem as people need to depend on brave core anyway.

[codefromthecrypt]

I chose Callable and Runable as they are pre Java 8 interfaces. So people can already use closures if they run on Java8 but Brave itself does not have to depend on Java 8. I think we could define a custom interface like Callable but without "throws Exception". If you prefer this then I can change the PR. Typically I try to avoid creating new interfaces if there is an existing one as people then depend on something proprietary. In case of brave this should be no problem as people need to depend on brave core anyway.

I do understand the general motivation, it is just that the context here is lambda, hence the previous comment about applicability. My concerns with this PR are -- we are looking to add a feature that no-one else has expressing interest in -- we'd end up with follow-up change due to the other functional lambda types like suppliers and consumers -- there's no need to do this by changing LocalTracer as it can be done via composition With normal composition, and a reference to a local tracer, we can use java 8 types directly vs awkwardly trying to fit JRE 6 types to functions, suppliers, and consumers. ex. FunctionalSpan { // initialized via a builder or lots of parameters String name; LocalTracer tracer; wrap(Function input) wrap(Consumer consumer) } Gut feel is that when there's a compelling type, others will express interest. Change with multiple consumers is the best kind of change to merge. Mind banging on this via composition until there's more interest? I'd certainly appreciate less code to maintain as frankly the current codebase is high effort and I'm in the middle of work trying to fix that.

[cschneider]

I dont think we need to support other lambda types. When you use lambdas java automatically detects if there is a compatble interface. Runnable supports the case when there is no return and Callable supports the case were there is one. Try it by using lambdas. It should work fine.

We could use composition but I fail to see how that makes things simpler.
Of course my change adds two methods to LocalTracer. Is that a problem?

[codefromthecrypt]

I've already mentioned my feedback against your design, and until there's someone else who wants it, I think it needs to cook. Restating eachothers' view in a loop won't resolve.

[codefromthecrypt]

I think the thing you are missing is that we should only change the public interface when it has support. Especially when others don't like the design. I often refer to the rule of three which helps decide. My suggestion is a workaround, which is that there's literally no need to change the interface to accomplish your goal. When there's wider support (ex multiple use cases or users), your workaround will be handy. Regardless, if more of the code used composition instead of tacking onto types or exposing things public, maintenance I'm currently attempting to do would take days not weeks. When in a hole.. stop digging, in other words. Why not consider java 8 as a modular addition until/unless it becomes so wanted that we have to change the type signatures and live with whatever choices they are? I really ought to copy this to brave as it does come up. Each person has their own favorite feature, yet we don't always remember that we can't do things like this unless there's others interested https://github.com/OpenFeign/feign/blob/master/HACKING.md

[codefromthecrypt]

ps I opened this to hopefully help with things like this in the future #287

on this change specifically, key point is that we needs others vouching for this feature because elaborating it is expensive.

Elaboration is beyond just choices around types to expose and whether or not to play animal sniffer games to allow them. More importantly, it adds error handling to the scope of local tracer.. something it currently doesn't have because it doesn't attempt to trace closures.

• this requires thought and testing, as if not, the span is sent before attaching an error to it!
• it may affect whether or not it even makes sense in localtracer (which has no policy around errors)
• look at ClientResponseInterceptor as an example
• some error practice is here Make it easy to code failure cases when using core annotations openzipkin.github.io#52

A lot of change that I've asked folks to elaborate on eventually gets in, we owe it to the project to not add debt and rather add things that take it away.

[cschneider]

That is fine. Thanks for explaining. Btw. I think it is very good to manage the API like you do. I see feature bloat a lot in apache projects and it is difficult to fight there.

[codefromthecrypt]

thanks for your patience with me, Christian!

[cschneider]

Do you think about something like this?

Instead of using LocalTracer like this:
Runnable:
localtracer.runInSpan("MyCompoent", "Myoperation", () -> someCoede())
Callable:
myResult = localtracer.runInSpan("MyCompoent", "Myoperation", () -> { someCode(); return "result"})

We would have a separate class like this:
FunctionalTracer tracer = new FunctionalTracer(brave, "MyComponent");

Runnable:
tracer.span("MyOperation", () -> someCoede());
Callable:
myResult = tracer.span("Myoperation", () -> { someCode(); return "result"})

[codefromthecrypt]

*We would have a separate class like this:* FunctionalTracer tracer = new FunctionalTracer(brave, "MyComponent"); Runnable: tracer.span("MyOperation", () -> someCoede()); Callable: myResult = tracer.span("Myoperation", () -> { someCode(); return "result"})

yep that's the idea (except not with a factory method/builder as that allows us to rewire it internally when the time comes) you could accept a Function for error formatter, too, for example. FunctionalTracer.create(brave).component("myfoo").errorFormatter(Throwable::getMessage);

[cschneider]

I agreed with Adrian to first prototype the closure support in my own project. So if you are interested in this watch this repository.

I will close the PR and open a new one once there is a good protype and some interest from the brave community for it.