Allow to configure EctoLogger through :telemetry.attach/4

[kamilkowalski]

For complex umbrella apps, having a global config for the :spandex_ecto library can be limiting - particularly when a different tracer is to be used depending on the repo.

This change allows passing configuration to the underlying EctoLogger module, like the :tracer option, through the config argument in :telemetry.attach/4.

Thanks to this change, the tracer could be configured when attaching SpandexEcto to telemetry events:

:telemetry.attach(
  "spandex-query-tracer",
  [:my_app, :repo_name, :query],
  &SpandexEcto.TelemetryAdapter.handle_event/4,
  tracer: MyApp.Tracer
)

It's backward compatible and shouldn't cause problems, the config argument was unused before.

Let me know what you think - I'd need to update docs and add some tests.

[GregMefford]

Sorry this took so long to respond to! 😅
I think this looks like a good idea and a good implementation - is this ready to merge or was there still something you were unsure about that caused you to leave it as a draft PR?

The only thing I can see is that we should add a note to the docs that demonstrates that this is possible and what the default is if you just set the config to nil like before.

[kamilkowalski]

The missing documentation is exactly why it's still a draft 😄 I'll update it and open a full PR.

[kamilkowalski]

And no worries - if it wasn't upvoted and I didn't remind you then it wasn't that big of a problem I guess. In our umbrella we've ended up using one tracer extracted to a monitoring app which all other umbrella apps depend on. I think the change is worth introducing anyway - global configuration of libraries isn't a good pattern in my opinion as it's often limiting, and the community is shifting away from it in favor of runtime config or the use Spandex.Tracer pattern of setting up named "instances" of modules.

[kamilkowalski]

I've changed the README so that it treats Ecto >=3 as the default - latest Ecto 2 release was in April 2019 and I don't think many people will configure SpandexEcto with Ecto 2. I've left Ecto 2-specific docs at the bottom in case anyone needs to refer to them still.

I mentioned tests in my PR description but I now see there are none - I started adding some but it's a lot of work and is beyond the scope of this PR. I can open a new one that implements tests once I finish.

[kamilkowalski]

I realized config options weren't documented anywhere, and also that this change allows specifying the database service name per repo - I've extended the docs. At our company SpandexEcto reports all databases under one name, since the configuration is global - it makes the Service Map in Datadog look like everything talks to one DB. This change would allow modifying the service name per-repo which would make so much sense.

[GregMefford]

Thanks for taking the time to work so hard on this! I didn't forget about it and I'm going to try to get it polished up and merged this weekend if I can find the time for it.

[kamilkowalski]

No worries, let me know if I can help with the polishing - after all I'm Polish!

[kamilkowalski]

Sorry, couldn't help myself.

[ischepin]

Hello guys, would really love to see this change merged, as I've being doing something similar in my fork. This obviously looks more polished than mine.

[ggiill]

+1 - Also running into this issue! Thanks @kamilkowalski.

[ggiill]

@GregMefford - What still needs to be polished? Happy to work on this.

[ggiill]

@kamilkowalski See my Configuration section suggestions below:

---

Configuration

Configure SpandexEcto globally in your application config:

# config/config.exs

config :spandex_ecto, SpandexEcto.EctoLogger,
  service: :ecto, # Optional
  tracer: MyApp.Tracer, # Required

Then attach it to your repository's telemetry events:

# lib/my_app/application.ex

:ok = :telemetry.attach(
  "spandex-query-tracer",
  # this should match your repo's telemetry prefix
  [:my_app, :repo, :query],
  &SpandexEcto.TelemetryAdapter.handle_event/4,
  nil
)

Or override the global config (useful for projects with multiple Ecto repos):

# lib/my_app/other_repo.ex

defmodule MyApp.OtherRepo do
  use Ecto.Repo,
    otp_app: :my_app,
    adapter: Ecto.Adapters.Postgres
end

# lib/my_app/config.exs

config :my_app, MyApp.OtherRepo
  telemetry_prefix: [:my_app, :other_repo],
  # other configs

# lib/my_app/application.ex

:ok = :telemetry.attach(
  "spandex-query-tracer-other-repo",
  [:my_app, :other_repo, :query],
  &SpandexEcto.TelemetryAdapter.handle_event/4,
  # this config will override the global config
  service: :other_db,
  tracer: MyApp.OtherRepoTracer # maybe this tracer adds custom metadata/tags
)

NOTE: If you are upgrading from Ecto 2, make sure to remove the loggers
entry from your configuration after adding :telemetry.attach/4.

[kamilkowalski]

@ggiill thanks for your suggestions - I've added matching :telemetry.attach/4 on :ok and split into two cases - without and with overrides to the global configuration. Since we weren't showing how to set up MyApp.Repo in the former case, I didn't find it useful in the latter - if someone's looking for Ecto tracing, they know how to set up a repo - so I skipped that part.

[GregMefford]

Sorry this sat idle for so long! I think it's a nice improvement, and I'm glad it was possible to do this in a non-breaking way!
Thanks again, @kamilkowalski and also @ggiill! ❤️ 🚀