-
Notifications
You must be signed in to change notification settings - Fork 117
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for dry-monitor events #924
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
--- | ||
bump: "patch" | ||
type: "add" | ||
--- | ||
|
||
`dry-monitor` events are now supported. There's also native support for `rom-sql` instrumentation events if they're configured. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
source 'https://rubygems.org' | ||
|
||
gem "dry-monitor", "~> 1.0.1" | ||
|
||
gemspec :path => '../' |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
# frozen_string_literal: true | ||
|
||
module Appsignal | ||
class EventFormatter | ||
module Rom | ||
class SqlFormatter | ||
def format(payload) | ||
["query.#{payload[:name]}", payload[:query], SQL_BODY_FORMAT] | ||
end | ||
end | ||
end | ||
end | ||
end | ||
|
||
Appsignal::EventFormatter.register( | ||
"sql.dry", | ||
Appsignal::EventFormatter::Rom::SqlFormatter | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
# frozen_string_literal: true | ||
|
||
module Appsignal | ||
class Hooks | ||
# @api private | ||
class DryMonitorHook < Appsignal::Hooks::Hook | ||
register :dry_monitor | ||
|
||
def dependencies_present? | ||
defined?(::Dry::Monitor::Notifications) | ||
end | ||
|
||
def install | ||
require "appsignal/integrations/dry_monitor" | ||
|
||
::Dry::Monitor::Notifications.send(:prepend, Appsignal::Integrations::DryMonitorIntegration) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I recall correctly (I was taking a look at this yesterday with Luismi) users can create several |
||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,22 @@ | ||||||
# frozen_string_literal: true | ||||||
|
||||||
module Appsignal | ||||||
module Integrations | ||||||
module DryMonitorIntegration | ||||||
def instrument(event_id, payload = {}, &block) | ||||||
Appsignal::Transaction.current.start_event | ||||||
|
||||||
super | ||||||
ensure | ||||||
title, body, body_format = Appsignal::EventFormatter.format("#{event_id}.dry", payload) | ||||||
|
||||||
Appsignal::Transaction.current.finish_event( | ||||||
title || event_id.to_s, | ||||||
title, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the formatted event test, both
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We might also want to provide a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wouldn't add dry to the event category. It's the thing that reports it, but no one cares about that detail. |
||||||
body, | ||||||
body_format | ||||||
) | ||||||
end | ||||||
end | ||||||
end | ||||||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
# frozen_string_literal: true | ||
|
||
describe Appsignal::EventFormatter::Rom::SqlFormatter do | ||
let(:klass) { described_class } | ||
let(:formatter) { klass.new } | ||
|
||
it "registers the sql event formatter" do | ||
expect(Appsignal::EventFormatter.registered?("sql.dry", klass)).to be_truthy | ||
end | ||
|
||
describe "#format" do | ||
let(:payload) do | ||
{ | ||
:name => "postgres", | ||
:query => "SELECT * FROM users" | ||
} | ||
end | ||
subject { formatter.format(payload) } | ||
|
||
it { is_expected.to eq ["query.postgres", "SELECT * FROM users", 1] } | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,104 @@ | ||
# frozen_string_literal: true | ||
|
||
if DependencyHelper.dry_monitor_present? | ||
require "dry-monitor" | ||
|
||
describe Appsignal::Hooks::DryMonitorHook do | ||
describe "#dependencies_present?" do | ||
subject { described_class.new.dependencies_present? } | ||
|
||
context "when Dry::Monitor::Notifications constant is found" do | ||
before { stub_const "Dry::Monitor::Notifications", Class.new } | ||
|
||
it { is_expected.to be_truthy } | ||
end | ||
|
||
context "when Dry::Monitor::Notifications constant is not found" do | ||
before { hide_const "Dry::Monitor::Notifications" } | ||
|
||
it { is_expected.to be_falsy } | ||
end | ||
end | ||
end | ||
|
||
describe "#install" do | ||
it "installs the dry-monitor hook" do | ||
start_agent | ||
|
||
expect(Dry::Monitor::Notifications.included_modules).to include( | ||
Appsignal::Integrations::DryMonitorIntegration | ||
) | ||
end | ||
end | ||
|
||
describe "Dry Monitor Integration" do | ||
before :context do | ||
start_agent | ||
end | ||
|
||
let!(:transaction) do | ||
Appsignal::Transaction.create("uuid", Appsignal::Transaction::HTTP_REQUEST, "test") | ||
end | ||
|
||
let(:notifications) { Dry::Monitor::Notifications.new(:test) } | ||
|
||
context "when is a dry-sql event" do | ||
let(:event_id) { :sql } | ||
let(:payload) do | ||
{ | ||
:name => "postgres", | ||
:query => "SELECT * FROM users" | ||
} | ||
end | ||
|
||
it "creates an sql event" do | ||
notifications.instrument(event_id, payload) | ||
expect(transaction.to_h["events"]).to match([ | ||
{ | ||
"allocation_count" => kind_of(Integer), | ||
"body" => "SELECT * FROM users", | ||
"body_format" => Appsignal::EventFormatter::SQL_BODY_FORMAT, | ||
"child_allocation_count" => kind_of(Integer), | ||
"child_duration" => kind_of(Float), | ||
"child_gc_duration" => kind_of(Float), | ||
"count" => 1, | ||
"duration" => kind_of(Float), | ||
"gc_duration" => kind_of(Float), | ||
"name" => "query.postgres", | ||
"start" => kind_of(Float), | ||
"title" => "query.postgres" | ||
} | ||
]) | ||
end | ||
end | ||
|
||
context "when is an unregistered formatter event" do | ||
let(:event_id) { :foo } | ||
let(:payload) do | ||
{ | ||
:name => "foo" | ||
} | ||
end | ||
|
||
it "creates a generic event" do | ||
notifications.instrument(event_id, payload) | ||
expect(transaction.to_h["events"]).to match([ | ||
{ | ||
"allocation_count" => kind_of(Integer), | ||
"body" => "", | ||
"body_format" => Appsignal::EventFormatter::DEFAULT, | ||
"child_allocation_count" => kind_of(Integer), | ||
"child_duration" => kind_of(Float), | ||
"child_gc_duration" => kind_of(Float), | ||
"count" => 1, | ||
"duration" => kind_of(Float), | ||
"gc_duration" => kind_of(Float), | ||
"name" => "foo", | ||
"start" => kind_of(Float), | ||
"title" => "" | ||
} | ||
]) | ||
end | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do so, there's no way of automatically formatting the events
rom-sql
emits because they're called justsql
. The suffix is added in thedry-monitor
to add a minimum granularity.I'm going to propose a change upstream to make
rom-sql
emitsql.rom
events instead. As ActiveRecord does.