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

DisableErrSkip() not working as expected #207

Closed
pmlanger opened this issue May 19, 2023 · 6 comments · Fixed by #210
Closed

DisableErrSkip() not working as expected #207

pmlanger opened this issue May 19, 2023 · 6 comments · Fixed by #210

Comments

@pmlanger
Copy link
Contributor

Hello,
to avoid the driver: skip fast-path; continue as if unimplemented error being attached to spans, I added the DisableErrSkip() option to the otelsql.Wrap() call like this:

otelsql.Wrap(d, otelsql.WithSystem("mysql"), otelsql.DisableErrSkip())

However, this hasn't produced the result I expected, as the error was still added to the spans.
I think that this is because vendor/go.nhat.io/otelsql/driver.go in newConnConfig does not add the errorToSpanStatus function to t.errorToStatus (t being methodTracerImpl).
I have patched the lib to add it, and it looks like it's working now (spans are not marked as error any longer, using the option).
If you agree, I could provide a PR with that change.

Also, do you have any additional insights as to how I could circumvent the error to happen in the first place (using github.com/go-sql-driver/mysql v1.7.1 with MySQL v8)?

Thanks a lot!

@nhatthm
Copy link
Owner

nhatthm commented May 20, 2023

Hey @pmlanger,

Thanks for opening the issue.

I think that this is because vendor/go.nhat.io/otelsql/driver.go in newConnConfig does not add the errorToSpanStatus function to t.errorToStatus (t being methodTracerImpl). I have patched the lib to add it, and it looks like it's working now (spans are not marked as error any longer, using the option). If you agree, I could provide a PR with that change.

I think you are right 👍 please open a PR as I can't fix this by Monday. Otherwise, I will work on a fix next week.

Also, do you have any additional insights as to how I could circumvent the error to happen in the first place (using github.com/go-sql-driver/mysql v1.7.1 with MySQL v8)?

Thanks a lot!

I'm not aware of all the use cases in each driver implementation. If you post some examples of where it happens, I may be able to look into it.

@pmlanger
Copy link
Contributor Author

@nhatthm Oh, you beat me by 5 minutes there :-)
Anyway, maybe it's worth looking at this: #210 since I have also added a test case.

@nhatthm
Copy link
Owner

nhatthm commented May 22, 2023

@pmlanger :D can you rebase with master then?

@pmlanger
Copy link
Contributor Author

@nhatthm Will do!

@pmlanger
Copy link
Contributor Author

I'm not aware of all the use cases in each driver implementation. If you post some examples of where it happens, I may be able to look into it.

Just to follow up on this: I have tracked it down to this line in go-sql-driver/mysql. Setting the InterpolateParams config to true makes the error disappear.

@nhatthm
Copy link
Owner

nhatthm commented May 22, 2023

Nice!

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.

2 participants