Skip to content
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 exec_params to profiled pg methods #317

Merged
merged 1 commit into from
May 30, 2024

Conversation

codez
Copy link
Contributor

@codez codez commented May 28, 2024

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
Copy link
Member

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

@codez
Copy link
Contributor Author

codez commented May 29, 2024

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 codez force-pushed the pg_exec_params branch from 2e3f059 to 905d9d6 Compare May 30, 2024 06:26
@codez
Copy link
Contributor Author

codez commented May 30, 2024

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
Copy link
Member

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.

@SamSaffron SamSaffron merged commit 99760a7 into discourse:main May 30, 2024
11 of 14 checks passed
@codez
Copy link
Contributor Author

codez commented May 30, 2024

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
Copy link
Member

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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants