Allow for greater configurability

[irl-segfault]

trafficstars

Problem Statement

Hi! Thanks for this great solution. Current issue is sheer volume of spans for requests that result in many DB queries. Here is a small fraction of spans emitted by a traced request, for example.

Concretely, my issues are

1. I've set DisableSkipErr to true, however the spans representing the skipped operations (in this case, the sql.conn.Query spans) are still recorded. Given that the operation is skipped, I don't see much use for these spans and would love an option to omit them from tracing entirely

2. I don't want the sql.conn.reset_session spans. Would be great to have an option to omit these.

3. I am debating whether or not I find the sql.conn.prepare spans useful. I think I would like an option to omit them as well.

In short, I really just want a single span per query/statement, in this case it would be the sql.stmt.query spans. I know many others may want finer grained detail, but in my case the utility of this library is often just to see the queries themselves as well as the timing -- the other spans are more useful in cases where my DB is having issues, in which case I would likely just roll out a quick config change to enable everything and begin debugging there.

Proposed Solution

Additional config values to

• OmitSkipErr value to omit a span with a skipped error completely (if DisableSkipErr is also set to `true

• OmitResetSession to omit sql.conn.reset_session spans

• OmitConnPrepare to omit sql.conn.prepare spans.

Alternatives

the alternative would be to do nothing and not give fined grained control over what's traced.

Prior Art

existing config values such as Ping and DisableErrSkip

[irl-segfault]

I'm willing to put up the PR here if it would be accepted :) Thanks

[XSAM]

Hi @irl-segfault, Thanks for raising this issue.

I've set DisableSkipErr to true, however the spans representing the skipped operations (in this case, the sql.conn.Query spans) are still recorded.

DisableErrSkip only suppresses driver.ErrSkip errors in spans. It has nothing to do with the query.

https://github.com/XSAM/otelsql/blob/e07a9d9581eaa3ca3ae47363274944d73eeacbcb/utils.go#L35-L39

Do you mean DisableQuery?

---

OmitSkipErr value to omit a span with a skipped error completely (if DisableSkipErr is also set to `true

I am not sure about this. Would you like to describe more about the use case?

OmitResetSession to omit sql.conn.reset_session spans OmitConnPrepare to omit sql.conn.prepare spans.

I prefer using a name like DisableResetSession to keep consistent with the existing config like DisableErrSkip, though I like the idea of omitting spans that you do not need.

Feel free to submit a PR if you are still interested.

[irl-segfault]

Basically, I'm just not sure when it's ever useful to see the actual span for the ErrSkip operation (fast path disabled thing). The extra span there just clogs up the view of the trace. Yes, it's nice that DisableErrSkip suppresses the actual error attached to the span, but the span still exists. I'm arguing it should be configurable to omit that span entirely.

So the general idea is that, as a user of this library, I may want only a subset of the spans -- specifically, I'm most interested in the Query/Txn/Stmt and Commit spans. Not generally interested in the sq.conn.prepare, sql.conn.reset_session and the sql.conn.query spans (which are skipped).

I think the config params should be called OmitX to specify that they will not appear in the trace. DisableSkipErr specifically refers to the error attached to that operation's span, so I like OmitX to indicate that the entire span is dropped.

The impetus for this whole argument is that this library is a really convenient way to trace the bulk of SQL interactions, inclouding capturing the query. In a steady state, this is fine and really all I want. If my DB starts going wonky, maybe then I'd be interested in the sq.conn.prepare, sql.conn.reset_session and the skipped sql.conn.query spans, but generally these impair the readability of my traces when such wonkiness is not occuring.

[XSAM]

I think the config params should be called OmitX to specify that they will not appear in the trace. DisableSkipErr specifically refers to the error attached to that operation's span, so I like OmitX to indicate that the entire span is dropped.

You are right. OmitX seems appropriate than DisableX in this scenario. 👍

The span will be created whether there is a SkipErr or not, SkipErr is an attachment for the span. We need options to control the span generation in certain operations to omit specific spans.

IIUC, we need options like these:

• OmitConnPrepare
• OmitConnResetSession
• OmitConnQuery
• OmitStmtQuery

[irl-segfault]

yeah! I'll put up a PR in the coming week when I find some free time. Thanks.

[grace-stytch]

+1 We've been running into a similar issue, the volume of spans has been a lot with only some providing value currently. @irl-segfault do you have a branch in progress? If so we'd be happy to see if we can help make some progress otherwise we can try to put up a PR for the solution outlined.

[XSAM]

@grace-stytch Feel free to submit a PR if you are also interested. 🚀

[mniak]

@XSAM I created a Pull Request (#87) implementing the OmitConnResetSession option. For the other options I think it would be better to not skip the spans, but to join into one big span instead since the complete skipping would leave gaps not accounted for. Bad for diagnostics.

[irl-segfault]

@mniak how would you join spans into a larger span? Which specific spans would be aggregated? IMO, that defeats the purpose of intentionally omitting spans like sql.conn.reset_session which ultimately do not add value in 99% of cases. In the event that you do feel the gaps are causing issues or you do need insight into sql.conn.reset_session timing, maybe the action to take would be to re-enable that span tracing. Just some thoughts :)

[XSAM]

I have created a PR to address this issue. It brings more span options to suppress the creation of spans that I think might be annoying.

@irl-segfault PTAL