Add Postgres Integration #2054
Add Postgres Integration #2054
Conversation
| def self.included(base) | ||
| base.prepend(InstanceMethods) | ||
| end |
There was a problem hiding this comment.
@delner I've seen this pattern before and I'm not sure why we do this (include to then prepend).
Do you know of any reason to not directly prepend our instrumentation module?
There was a problem hiding this comment.
It necessary when patching a blend of class and instance methods. As a matter of convenience, it also removes the ambiguity of when to include and when to prepend by encapsulating the correct behavior in the module. Moreover, it means you can also apply multiple modules in one include call. See Workers::Polling as an example.
Here, it's not strictly necessary, and I'm agnostic to whether we repeat the pattern or not.
| Tracing.trace(Ext::SPAN_EXEC, service: service) do |span| | ||
| span.resource = sql | ||
| span.type = Tracing::Metadata::Ext::SQL::TYPE | ||
| span.service = Ext::DEFAULT_PEER_SERVICE_NAME |
There was a problem hiding this comment.
Minor change: let's try to move as many values to the trace method call arguments (e.g. type be passed a a keyword argument to #trace for example).
This reduces the amount of times the span is mutated, and can allow for better introspection of the Span at initialization time if any parties are interested in inspecting it (e.g. sampling could do that in the future).
There's a minor performance improvement as well, as we save a method call (e.g. span.type = Tracing::Metadata::Ext::SQL::TYPE).
There was a problem hiding this comment.
Especially a good point regarding sampling behavior.
jennchenn
force-pushed
the
jenn/postgres-integration
branch
from
f8b7bd5
to
a06bb11
Compare
jennchenn
force-pushed
the
jenn/postgres-integration
branch
from
42b2532
to
4fd1be9
Compare
Codecov Report
@@ Coverage Diff @@
## master #2054 +/- ##
==========================================
+ Coverage 97.47% 97.51% +0.03%
==========================================
Files 1035 1042 +7
Lines 52663 53384 +721
==========================================
+ Hits 51334 52055 +721
Misses 1329 1329
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
delner
added
integrations
|
This just got released as part of v1.2.0 -- @jennchenn your first PR is now in the hands of customers! Good job! :) |
|
Hi @jennchenn, does this integration only work for explicit calls to pg gem or will it also provide a dependency breakdown when calling pg through activerecord on rails environments? |
|
@pedrocunha, calls invoking the pg gem from any source will be instrumented, including indirect calls from Is there any issue you see today with the |
Description
This PR adds new instrumentation for the
pggem (>=v0.18.4). [GitHub Repo] [Ruby Gem]Support was added for the following methods:
#exec#exec_params#exec_prepared#async_exec#async_exec_params(>=v1.1.0)#async_exec_prepared(>=v1.1.0)#sync_exec(>=v1.1.0)#sync_exec_params(>=v1.1.0)#sync_exec_prepared(>=v1.1.0)Examples
These changes were tested in a sample rails application that make queries to a postgres database directly using the
pggem. Some sample traces are shown below:Performance Benchmarks
The time it takes to execute a query using the
pggem when the tracer is enabled vs. when it is not was compared. The results for each of the supported methods are shown in the table below for the following queries:SELECT 1;,SELECT * FROM <table with 100 rows>;andSELECT * FROM <table with 1000 rows>;With tracing disabled:
With tracing enabled:
Though there is a performance difference between the two, the overhead from having the tracer enabled for the
pggem seems to be on the same order as with other gems.