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

xray.SqlContext duplicates all non-trivial SQL in trace #429

Open
fdegiuli opened this issue Nov 2, 2023 · 2 comments
Open

xray.SqlContext duplicates all non-trivial SQL in trace #429

fdegiuli opened this issue Nov 2, 2023 · 2 comments

Comments

@fdegiuli
Copy link

fdegiuli commented Nov 2, 2023

The connections provided by the go MySQL driver will return ErrSkip to any SQL query with parameters unless the interpolateParams option is explicitly set to true--it is false by default. (See https://github.com/go-sql-driver/mysql/blob/master/connection.go#L368)

This SDK's implementation of SqlContext creates a subsegment for both the original aborted non-call (which returns ErrSkip and is never executed) and the following actual call to the database.

This causes the majority of SQL queries to show up twice in the trace; one with skip fast-path; continue as if unimplemented appended to the query, and then another with just the query text.

Ideally SqlContext would avoid creating a subsegment when it receives ErrSkip

See https://github.com/aws/aws-xray-sdk-go/blob/master/xray/sql_context.go#L238 if my explanation is confusing

@fdegiuli
Copy link
Author

fdegiuli commented Nov 2, 2023

I'm happy to put up an MR if we think this is worth doing.

My thought was to introduce a segment.Abort() method, which simply removes the segment from its parent and discards it. The Capture function could then look for a sentinel error and abort the segment instead of closing it.

Let me know if that seems reasonable or if there's a better way.

@wangzlei
Copy link
Contributor

wangzlei commented Nov 17, 2023

Thanks for raising this case.
Trace, as a monitoring tool, records the conditions that occur during program execution, much like logs and metrics. Therefore, similar to retries, Sql queries with ErrSkip should also be logged. Simply deleting ErrSkip subsegments is not a generic solution for every user. If presenting ErrSkip alongside successful SQL queries creates a perception of duplicates for customers, we should find a way to enhance the user experience in this area. At the very least, we need to ensure that ErrSkip subsegments contain clear indications, preventing customers from easily misunderstanding the situation.

Could you help provide the trace raw data and a trace graph screenshot with ErroSkip and successful queries? We will investigate whether it could be improved.

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

No branches or pull requests

2 participants