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 config option to trace raw tx.Statement.SQL.String() without applying tx.Dialector.Explain #14

Closed
chradcliffe opened this issue Sep 14, 2023 · 1 comment · Fixed by #15
Assignees

Comments

@chradcliffe
Copy link
Contributor

Describe the feature

I'd like to add a config option to skip the tx.Dialector.Explain step step when tracing the query (i.e. just record the result of tx.Statement.SQL.String()). This can either be its own option or it could replace the WithoutQueryVariables() option, which has some significant performance issues for some uses (see below). I'm not sure that there's a good use case for WithoutQueryVariables() that wouldn't be solved with the proposed option.

I have a working version of the option in a fork I created here: master...tonalfitness:gorm-opentelemetry:master

If you're open to this request, I can open a PR for this change.

Motivation

I'm using this plugin in a production environment, configured with the WithoutQueryVariables() option. We originally configured the overall tracing with a relatively low sampling rate (10%) and there wasn't a noticeable change in performance. When we switch to 100% tracing (in order to tail sample our traces), we noticed a very large increase in latency and high CPU utilization that we traced back to the use of this plugin (by disabling it and re-testing).

We traced the actual performance issue to the use of tx.Dialector.Explain, which substitutes the actual parameters into the query string. This results in a lot of string copies, which I think makes sense given what it's doing. We use the WithoutQueryVariables() option, but all that does is substitute ? for the variable values -- it still does the string interpolation.

My preliminary performance testing of the forked version mentioned above shows that the high CPU utilization and latency goes away but we still get a useful representation of the SQL query used.

Related Issues

@zstone12
Copy link
Collaborator

@chradcliffe Pull Requests are highly welcome. In some scenarios, specific query parameters are also required, so I think the current approach of using options for modification is very appropriate.

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

Successfully merging a pull request may close this issue.

3 participants