Bring back Spandex.TraceDecorator

[dnlserrano]

This is a preliminary version of what we discussed over at the #spandex channel in the elixir-lang Slack.

It basically is a resurrection of what @zachdaniel had implemented (and deprecated) in the past using the decorator library. Me, @dcaixinha and @amalbuquerque were looking into this in a mob-programming fashion (❤️💚💛💙💜) and found some problems with it. Maybe it'd be nice to have the lib decorate stuff without the aid of the decorator library (although we still find it very useful!!), simply because it injects a __using__ and that has the disadvantage of not allowing any other __using__ statements to be defined in the context of whoever uses Spandex.TraceDecorator.

Anyway, I think this is a good starting point that helped us in migrating from 1.3.3 to 2.3.0. 😄

I'd love to hear your thoughts on some things in particular:

• documentation
• comments
• config :spandex, :decorators, tracer: MyApp.Tracer
• conditional compilation of Spandex.TraceDecorator

I think this last one should be done before merging, I can help once I get back and have some feedback from you guys. I was looking at mix's elixirc_paths, but I don't see an elegant way of excluding lib/spandex/trace_decorator.ex. I'll dig deeper.

/cc @zachdaniel @GregMefford

[sourcelevel-bot[bot]]

Hello, @dnlserrano! This is your first Pull Request that will be reviewed by Ebert, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information.

[dnlserrano]

Neat, automation! I'll address when I have some more time! 🤓

[coveralls]

Pull Request Test Coverage Report for Build 380

• 0 of 0 changed or added relevant lines in 0 files are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage remained the same at 82.329%

---

Files with Coverage Reduction	New Missed Lines	%
lib/plug/end_trace.ex	1	91.67%
<!--	Total:	1		-->

Totals
Change from base Build 364:	0.0%
Covered Lines:	205
Relevant Lines:	249

---

💛 - Coveralls

[zachdaniel]

As far as the methodology behind conditionally compiling modules, the way I've seen it done isn't pretty but it gets the job done :D

if Application.get_env(:something, :something_else) do
  defmodule MyModule do

  end
end

[zachdaniel]

I think it might be suboptimal to have a single tracer used for all decorators. Instead, it might be better instead if you could use MyApp.Tracer in any module, gaining access to a decorator for that tracer. However, this becomes a relatively dense series of macros within a module defined via a use in the first place. What if we were to set it up where either a tracer was accepted as an opt or as a module attribute?

That could support separate tracers (which is an important thing to retain support for).

So something like this could work:

defmodule Something do
  @tracer MyApp.Tracer

  @decorate span()
  def foo() do

  end

  @decorate span(tracer: MyApp.OtherTracer)
  def foo() do

  end
end

As a counter to my own argument, I'd be willing to bet that 95% of people aren't using multiple tracers, so it might be a better idea to have three tiers of configuration, where we first check an explicit opt, then check a module attribute, then check application env.

Aside from all of that, my only other feedback is that we should make the compilation of this dependent on the inclusion of decorators at all. Mix allows you to specify optional dependencies that won't be included in things that depend on spandex by default. More docs here: https://hexdocs.pm/mix/Mix.Tasks.Deps.html

Then, before compiling the decorator code we check for compilation of the decorator code, similar to the way ecto does for postgrex: https://github.com/elixir-ecto/ecto/blob/master/lib/ecto/adapters/postgres/connection.ex#L1

[dnlserrano]

I think it might be suboptimal to have a single tracer used for all decorators. Instead, it might be better instead if you could use MyApp.Tracer in any module, gaining access to a decorator for that tracer.

Cool, I'll try and give this a go.

Aside from all of that, my only other feedback is that we should make the compilation of this dependent on the inclusion of decorators at all. Mix allows you to specify optional dependencies that won't be included in things that depend on spandex by default. More docs here: https://hexdocs.pm/mix/Mix.Tasks.Deps.html

From what you're suggesting, we should have a separate spandex_decorators library, as we had previously discussed over the #spandex Slack channel. Is that it?

Then, before compiling the decorator code we check for compilation of the decorator code, similar to the way ecto does for postgrex: https://github.com/elixir-ecto/ecto/blob/master/lib/ecto/adapters/postgres/connection.ex#L1

So, I'd have a library spandex_decorators, optionally included in spandex. That library would define a way of creating trace decorators. In spandex, I check if the code for spandex_decorators is loaded. If it is, then we define the decorator people should use by leveraging the code provided by spandex_decorators?

It seems a bit convoluted, if I understand it correctly, TBH. 😅 But this is probably just my inner-noob talking.

[zachdaniel]

@dnlserrano not quite. What we do is we tell people "if you want to use decorators, add {:decorators, "~> 1.0} to your mixfile" (or whichever version they prefer). Then, we don't compile our decorator module, unless they've added the decorators dependency (making the entire dependency optional, instead of just the compilation of the module).

[zachdaniel]

I also think it would probably be better to go with the second kind of configuration I mentioned, allowing it to be passed as an arg, and defaulting to a module attribute and/or application env. Curious to get @GregMefford's thoughts on both recommendations.

[GregMefford]

First of all, I want to say another huge thanks to @dnlserrano for taking the time and energy to put together this PR and go through a bunch of back-and-forth about it. ❤️ 🚀 ✨ As you know, it's a potentially-tricky feature and we want to make sure we get it as-close-to-right as we can before we merge it. So I hope you won't be discouraged by all of our comments and details.

As far as configuring the Tracer module to use for your decorators, I always ask myself what is going to be the least-surprising and least-magical solution. Using decorators at all is already pretty magical to me, but I think the industry has decided that tracing and other logging-type things is potentially the one case where decorators make a lot of sense, so I'm comfortable adding it as a feature as long as we can make it reliable. The last think people want is for their tracing code to cause application failures.

@zachdaniel, I'm not a big fan of using a module attribute to set the default tracer for that module because as a developer, you're setting an option in your business logic module, but you can't see the code that's using it since it's buried in Spadex's Decorators module. As far as I understand, the purpose of the Tracer is so that you don't have to individually specify all the trace/span options because some of them are defaulted by the Tracer. The same could be accomplished, for example, like this:

defmodule Something do
  use Spandex.Decorators

  @default_trace_opts [
    service: :something,
    type: :web,
    tracer: MyApp.Tracer
  ]

  @decorate span(@default_trace_opts)
  def foo(), do: :ok

  @decorate span(@default_trace_opts ++ [tracer: MyApp.OtherTracer])
  def bar(), do: :ok
end

Sure, it's a bit more ugly this way, but it's more explicit because you're constructing your defaults yourself, however you want, instead of using a magic attribute that we control and you can't use for your own code.

We could also clean up the ++ by defining a span/2 that does the merging behind the scenes, so you could do this instead:

  @decorate span(tracer: MyApp.OtherTracer, @default_trace_opts)
  def bar(), do: :ok

It's a bummer that the decorator library doesn't seem to have a way to pass a parameter into the __using__ macro (at least as far as I can tell), because we would ideally be able to just do:

defmodule Something do
  use Spandex.Decorators, tracer: MyApp.Tracer
  # ...
end

[GregMefford]

I also took some time to look into how other similar libraries are doing it. AppSignal is using decorator and NewRelic has rolled their own. I'm honestly not sure which one scares me more because the code in the AppSignal implementation looks more robust, but if we lean on decorator, then it's not our problem to maintain it, aside from potentially contributing fixes to that library if we have any.

[zachdaniel]

I think you are right about all of that. Rolling our own might be the only way to get the kind of confit you’re talking about, but it’s also way better to read and avoids lots of confusion I think. Let’s look into rolling our own (or just forking decorator and adding support for compile time args to the using)

_{Sent with GitHawk}

[GregMefford]

I think the best path forward is to get in touch with Arjan about whether he'd be open to having a way to parameterize the __using__ macro and if he's thought about how that work work yet. I don't particularly want to maintain a fork, and I bet others using the library would like that feature too if we can figure it out.

I can't commit to finding time to work on that anytime soon myself, just because there are only so many hours in the day, but I may eventually get around to it if someone else doesn't first. 😅

[zachdaniel]

@GregMefford yeah, finding out from him if he would be interested in accepting that feature would be nice :)

[asummers]

My experience working with him in the past was that he was amenable to sane changes, so he'll likely be open to it.

[zachdaniel]

Alright, after evaluation, I think there is a way to make this work that doesn't involve having to change the decorators library, it just requires some weird internals.

Inside the __using__ for tracer, we will define a defmacro __using__(_) that brings in decorators for specifically that tracer. Then you just say use MyApp.Tracer and that is what gives you access to @decorate span(_). The limitation there is that you can only use one tracer per module via decorators though.... I'm not sure if thats the best. @dnlserrano I'm going to make a PR to your branch with those changes.

[zachdaniel]

Well, I actually won't have time to do that until tonight/tomorrow morning. @dnlserrano if you agree with the above approach and want to add the necessary macros for that feel free.

[GregMefford]

The limitation there is that you can only use one tracer per module via decorators though.... I'm not sure if thats the best.

I think you could still override for a particular function decorator with @decorator span(tracer: MyApp.AnotherTracer), right? I think that's a reasonable limitation. A similar idea I was rolling around in my head was if we added a defmodule Decorators inside Tracer's __using__ macro, so that in your traced module, you'd do use MyApp.Tracer.Decorators to make it more clear that you're going to include decorators as opposed to something else from the Tracer module itself. The part I don't love about that idea is that it's magical because it's not obvious where that Decorators module is actually defined. Same problem as with the Spandex.Tracer module vs. the MyApp.Tracer module though, I guess.

[zachdaniel]

@GregMefford I'm not entirely sure that you can override it that way, actually. It would amount to MyApp.Tracer.span(tracer: SomethingElse), which I don't think will work.

[zachdaniel]

Okay, I think I've changed my official stance on this. Lets start simple, and go from there:

1.) Make the decorator dependency optional 2.) Only compile our decorators module when that is present 3.) Make :tracer a required opt for the decorator function.

That would be a great base to start from. We can explore shortcuts and conveniences later.

WDYT?

[GregMefford]

Is there a reason to make tracer a required opt as opposed to allowing it to be defaulted to one global default based on application config? I think the latter would meet the needs of most users, and allowing it to be overridden on a per-decorator basis would cover the remaining need to support multiple tracers.

[zachdaniel]

Yeah, I can buy that :) so we’re basically back to the original implementation, except it’s conditional on decorators being present and tracer can be overridden.

[dnlserrano]

I'm trying to work on this now! Be back soon.

[dnlserrano]

Thanks for all the comments guys!

I pushed some changes that meet some of your requirements, still want to explore DRYing out the macro part a bit more. But I think the gist of it is there, maybe?

I look forward to hearing from you, and thanks for your patience with the noob over here! :wave::sweat_smile:

[zachdaniel]

Few more pieces of feedback. The things I'm commenting on were really just mistakes with the original implementation, not your work at all :) But we might as well get it ship shape before...shipping it.

[dnlserrano]

Pushed some more changes.

[zachdaniel]

I'll check out the macros issue, and get back to you :D

[zachdaniel]

Some proposed changes: https://github.com/dnlserrano/spandex/pull/1

[dnlserrano]

Now also updated with @zachdaniel's wonderful contribution! Let's merge ittttt... 🕊 maybe? 😛

By the way, I tested it running against a real Datadog instance, using the version of code at @zachdaniel's hash.

[zachdaniel]

If @GregMefford approves, let’s metge it.

[zachdaniel]

I’ll add something to the change log when we release.

[GregMefford]

Once I get the actual spandex_examples infrastructure finished so that it has some interesting API calls available, I’d like to add some decorators in there and shake out any bugs/problems before we merge. :thumbsup:

[zachdaniel]

Could you elaborate on the configuration issue? Since its not scoped to a tracer, it seems like they should be able to say:

config :spandex, :decorators,
  tracer: MyApp.Tracer

[dnlserrano]

I configure the decorators by doing

config :<my_otp_app>, Spandex.Test.Support.OtherTracer,
...

:spandex_test won’t work since that app doesn’t exist. :decorator on the other hand does, since that’s the app for the library we’re using for our decorators.

[zachdaniel]

Oh, I see this is just for configuring it in testing. Why can't that tracer have its otp_app set to spandex also? They can share an otp_app, because they are namespaced further by the name of their module.

[dnlserrano]

Yeah I guess I can do that. 🤓

[dnlserrano]

Now using :spandex OTP app, also added some clarification on the commit message.

I wonder if @GregMefford is OK with this approach. I empathise with his fear of people thinking this is some configuration done at the spandex level and not at the OTP app level. Not really sure how we can get around it though.

I'll wait for your feedback, guys! Thanks. 🙇

[GregMefford]

Yep, I like using :spandex as the app name in the tests better than :decorator. It's not great, but really that's only for our internal use anyway, so I don't think people will be confused. We will clarify how it's supposed to work when it's integrated in the spandex_example repo, which I envision will become the de-facto "living documentation" reference details about how to set things up the way we intend.

[GregMefford]

Ok, I've started working on trying this out in the spandex_example project and, as I guessed earlier, we get errors because the resource isn't being set:

datadog_1     | [ TRACE ] 2018-10-11 02:09:00 ERROR (receiver.go:249) - dropping trace reason: invalid span (SpanID:8157882088627400269): span.normalize: empty `Resource` (debug for more info), [service:"phoenix_backend" n...

I'm not really sure what to call the resource by default in the case of function decorators, but I'll poke around a bit and see if I can find a precedent in some of the other Datadog APM libraries.

[GregMefford]

Looks like resource defaults to name in the official Ruby implementation. That's good enough for me! 👍 🚀

I'll make a PR to spandex_datadog to default it there while sending out spans, since I think this is more of a Datadog requirement than something Spandex itself should enforce.

[zachdaniel]

Alright, there is some small feedback, like making that function private. I’ll merge this once that is done!

[GregMefford]

After applying that fix to spandex_datadog, I was able to verify that some basic function decorators do work! 🎉

I'm not really sure how to handle this situation though - if someone were to upgrade spandex to get the decorator feature, it would totally break their entire APM unless they also upgrade spandex_datadog. Should we go ahead and also set the resource to be the same as the name by default in this PR? That would probably be a good idea.

Also, as you can see in the screenshot, I think we should remove the Elixir. prefix from the module name that gets generated by default.

[GregMefford]

It looks like I was able to push commits directly to this PR branch 🎉 LMK what you think about those tweaks.

[dnlserrano]

Looks great!! 😍 Thanks for pushing these changes. ❤️

[zachdaniel]

I don't know if we want to do it at the options level for the decorator though. That means that any default configured resource will never be used. Right now, if you do:

config :tracer, MyApp.Tracer,
  resource: :some_resource

and then don't specify resource in the future it will use that resource. With these defaults it won't do that (specifically for decorators)

[GregMefford]

Ah right, I hadn't thought about configuring a default resource for the tracer like that. 🤔 I don't really see why you'd want to do that, but we should tweak this so it honors that default as well, only failing back to the name as the default after that. I might be able to get back to that late tonight, but if someone else gets to it first, feel free.

[zachdaniel]

I had confused resource and service in this context. It would not make sense to have a globally default resource. This should be good to go.