romul / newrelic.ex

NewRelic agent for Elixir
MIT License
64 stars 19 forks source link

Use phoenix and ecto instrumentation #7

Open blatyo opened 7 years ago

blatyo commented 7 years ago

Phoenix instrumentation is mentioned here.

Ecto instrumentation is mentioned here. Search for :loggers.

wfgilman commented 7 years ago

I've been looking into these two. I think there may be an issue with Ecto instrumentation because you are restricted to the data in the Ecto.LogEntry. You'd be missing out on the calling module.

For the Phoenix instrumentation, the approach this library takes seems much simpler. Instead of creating many event handlers for each action you want to instrument, you can pass it straight through to the collector from the controller via a plug.

blatyo commented 7 years ago

For the Phoenix part, it's actually pretty simple, because they already instrument parts of your app including the web requests, web sockets, and view rendering. So, you just provide a function to tie into those existing instrumentations. The plug version only records the web request time.

For the Ecto version, yes, you would be limited to the Ecto.LogEntry information. As far as I'm aware, that provides more information than what is provided currently with this library. What calling module are you referring to?

Also, it appears conn is passed around just so that new_relixir_transaction can be accessed. That would probably be a candidate for storage in the process dictionary, which would not require the information to be passed around. This is what Logger uses for it's metadata for example, so that you don't have to pass metadata to each log request.

wfgilman commented 7 years ago

Thanks for the color on the Phoenix instrumentation. I didn't realize that.

For Ecto, I'm looking to use this library separate from Phoenix, so I don't want to pass a conn around. I'd like to instrument my db functions which perform ETL and need something more general purpose - something like this:

  @doc """
  Records execution time of a function.
  """
  @spec record(mfa, atom) :: any
  def record({mod, fun, args}, scope) when is_atom(scope) do
    f = fn -> Kernel.apply(mod, fun, args) end
    {elapsed, result} = :timer.tc(f)
    name = "#{mod}.#{to_string(fun)}"
    :ok = NewRelic.Collector.record_value({name, {scope, name}}, elapsed)
    result
  end

By "calling module" I meant being able to access the meta data stored in the conn.private under new_relixir_transaction so I can associate the Ecto.LogEntry data with it.

I hadn't considered a process dictionary, I need to look into that.

blatyo commented 7 years ago

I agree, this library should be usable outside a phoenix context. It makes sense to be able to instrument any type of application.

wfgilman commented 7 years ago

I'm working on some modifications to it. I'll put it up in a couple of days.

wfgilman commented 7 years ago

I started working on a PR to add some of these features, but ended up having a hard time finding the documentation for the API that lhttpc was talking too. I also had trouble making sense of the transformation logic in the statman module. In short, I ended up re-writing the agent and statman modules and ultimately realized this was less a PR than a separate library. It's up on my git if anyone wants to take a look, called NewRelix. Maybe we can combine forces here.

blatyo commented 7 years ago

I looked and it looks like you implemented it as a new relic plugin API instead of using the APM API. That's workable, but from my perspective inferior, because new relic can show you this request took a long time, because the database call took a long time. That context gets lost when sending them as separate events.

As for the APM API, it's not publicly available. I believe it was reverse engineered from the ruby gem [1]. If you wanted to go that route, I'd recommend starting by looking at the tests first.

wfgilman commented 7 years ago

I see your point. I probably won't go down that road and reinvent the wheel here. From what I understand, native Elixir support a WIP at New Relic.