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

sql.conn.reset_session #7

Closed
DejeroDaniel opened this issue Apr 28, 2021 · 4 comments · Fixed by #13
Closed

sql.conn.reset_session #7

DejeroDaniel opened this issue Apr 28, 2021 · 4 comments · Fixed by #13

Comments

@DejeroDaniel
Copy link

Hey, I am using this in my application. Nice work!
An issue is, I am seeing constantly a sql.conn.reset_session span. Not sure what it is, but it seems to be happening on each Ping? Very spammy. Should this span be disabled when Ping's are disabled?

@XSAM
Copy link
Owner

XSAM commented Apr 29, 2021

@DejeroDaniel Thanks for raising this issue.

From the Go Doc about the SessionResetter:

ResetSession is called prior to executing a query on the connection
if the connection has been used before. If the driver returns ErrBadConn
the connection is discarded.

It will happen on a second or more calls no matter the call is Ping or Query. It's an actual operation that the driver will do and might help while debugging.

I not sure we should drop this reset_session span by default then use a flag to enable it or contrarily.

@seh @fsaintjacques @nvx Any suggestions on this one? 😄

@seh
Copy link

seh commented Apr 29, 2021

I haven't noticed these spans. If this is something the driver is performing involving network activity (or even something that might take a while if interacting with SQLite locally), I'd prefer to see it represented in the traces. If we'd consider filtering this one, I'd prefer that we treat the problem more generally, to avoid adding something to the exported interface for just this particular operation.

@DejeroDaniel
Copy link
Author

I use both ddtrace and otel, and when I am using gopkg.in/DataDog/dd-trace-go.v1/contrib/database/sql and gopkg.in/DataDog/dd-trace-go.v1/contrib/jmoiron/sqlx I don't see these spans at all. They must be filtering it in that library somewhere.

Right now I gave the db driver a different service name so I can filter these out in the dashboard, but it is pretty spammy.

Even if we think these are important info, I think the sql.conn.reset_session that are related to pings should go away if we have disabled ping in the SpanOptions.

@XSAM
Copy link
Owner

XSAM commented Apr 30, 2021

I think the sql.conn.reset_session that are related to pings should go away if we have disabled ping in the SpanOptions.

Definitely agree with that. Also, I think there is no SpanContext passed to the Ping function, so the sql.conn.reset_session span will separate from the Ping span.

This brings me the idea that we should not instrument a sql function by default if there is no Span in the Context since discrete spans might have little help for analysing what happens in the program. It can also address @DejeroDaniel 's concern.

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