search

discourse / prometheus_exporter

A framework for collecting and aggregating prometheus metrics
MIT License
525 stars 153 forks source link

Add exec_params to profiled pg methods #317

Closed codez closed 1 month ago

codez commented 1 month ago

ActiveRecord uses PG::Connection#exec_params for queries that are not cached (via `ActiveRecord::ConnectionAdapters::PostgreSQLAdapter#exec_no_cache / #execute_and_clear). As this method is not profiled, a substantial amount of queries, and hence DB time is missing from the prometheus instrumentation.

This PR adds the missing method to the list of profiled methods.

SamSaffron commented 1 month ago

oh my our test suite is broke. I do think we should add a test for this cause it is high risk

codez commented 1 month ago

I'm happy to add a test, but I already ran into problems installing the required version of libv8-node locally as it requires an old version of python. And when I saw the failing CI, well, ...

Let me know how to proceed. I think without this method the Postgres sql duration metric is not really meaningful, so I would really like to see this integrated.

Edit: I created #318 to fix the tests. Based on this, I would be able to add a test for this change here.

codez commented 1 month ago

In the current tests, there are only a couple of mocked calls that test the basic setup of the middleware and nothing that actually integrates with PG. The pg gem is not a development dependency. My approach would have been to add this dependency and create a test that actually calls the gem's method, in order to assert a correct integration. Probably this would require setting up a database as well (to avoid mocking internal pg methods).

So before I add all this stuff, I want to ask if this is really desired or if you see a more lightweight testing approach that still provides additional benefits?

SamSaffron commented 1 month ago

I think I will just accept for now. Looking through : https://github.com//blob/d072b21852865ecb84e6345df11d68eed50702bb/ext/pg_connection.c#L3347-L3366 its a safe patch it is not going to stack with other calls.

We may be missing a few more patches but we can deal with them later... https://github.com//blob/d072b21852865ecb84e6345df11d68eed50702bb/ext/pg_connection.c#L4530-L4542

Its a bit unfair to force tests here when mysql and other patches get away without them ... right way is adding all the dev dependencies but it is a big project.

codez commented 1 month ago

Ok, thanks for merging. Testing for all possible integrations is a huge amount of work indeed.

There are a lot of methods in PG::Connection! Although ActiveRecord only uses prepare and get_last_result additionally. I did not look into those in detail, but as they are clearly database related, I guess it might make sense to add them here as well. Shall I create a new PR?

SamSaffron commented 1 month ago

I would leave them out for now cause I worry a bit about having too much instrumentation. it is taking so long for you cause bundler has bugs sadly...