Add support for dry-monitor events

[luismiramirez]

New hook that adds transaction events every time a dry-monitor notifications instrumentation is called.

There's also specific support for rom-sql events emitted through dry-monitor.

Screenshot:

Closes: #910

[luismiramirez]

I'm not entirely into adding the .dry suffix to all dry-monitor notifications. The main reason to add it is to avoid having an event formatter called sql just for rom-sql

[unflxw]

@luismiramirez I'm not entirely into adding the .dry suffix to all dry-monitor notifications.

I do think it's valuable to be able to tell apart ActiveSupport notifications and Dry::Monitor notifications. Maybe instead of the .dry suffix, use something that couldn't ever appear in an ActiveSupport notification name? The "name" that an event formatter is registered with (or looked up by) does not need to be a string, so you could give them something like [:dry, "sql"] as a name?

[unflxw]

In the formatted event test, both title and name are set to "query.postgres", but in the test for the generic event one of them is left empty. Is that the intended behaviour? If not:

Suggested change

	title,
	title || event_id.to_s,

[unflxw]

We might also want to provide a .dry suffix for the resulting event category -- although we don't currently do this for ActiveSupport notifications.

[tombruijn]

I wouldn't add dry to the event category. It's the thing that reports it, but no one cares about that detail.

[tombruijn]

Suggested change

	"sql.dry",
	"sql.rom",

Let's name it for the gem it's formatting the events for.

We have a bunch of sql formatters that do very similar things and we can combine them one day, but this is a safer approach right now.

[unflxw]

But this has to match the prefix given to look for an event formatter when the dry-monitor instrumentation receives the event -- and this instrumentation is generic to all dry-monitor events (just like the activesupport notifications one is generic to all activesupport events)

[luismiramirez]

If we do so, there's no way of automatically formatting the events rom-sql emits because they're called just sql. The suffix is added in the dry-monitor to add a minimum granularity.

I'm going to propose a change upstream to make rom-sql emit sql.rom events instead. As ActiveRecord does.

[tombruijn]

I wouldn't add dry to the event category. It's the thing that reports it, but no one cares about that detail.

[tombruijn]

I know we went this route for the active support notifications integration because of performance issues, but I don't know if that's a problem here. We can instead subscribe to the wildcard * event name instead to receive events from the instrumentation and use the built-in method of listening for events.

[unflxw]

If I recall correctly (I was taking a look at this yesterday with Luismi) users can create several Dry::Monitor::Notifications instances, each being its own "notification hub" of sorts -- it's not a singleton like ActiveSupport::Notifications. This allows us to instrument all instances.

[unflxw]

Since it doesn't seem that the change that @luismiramirez proposed upstream is going to happen, how about merging the original implementation that @luismiramirez proposed? The one where we internally add .dry to dry notification events in order to be able to tell them apart.

(To be clear, the .dry suffix is only added in order to tell them apart in our internal EventFormatter -- the actual event sent to AppSignal wouldn't say .dry, if I recall correctly)

If not, then we may want to consider just closing this. :(

[tombruijn]

That approach is probably good enough, but it's been a while since I looked at this PR.

Do we have any links to issues/posts/discussions about this upstream change?

[tombruijn]

Needs rebase

[backlog-helper]

• This Pull Request needs more reviews. @jeffkreeftmeijer - (More info)

---

This is a message from the daily scheduled checks.

New issue guide | Backlog management | Rules | Feedback