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

Support custom database label for sql.db tracing #231

Closed
cgstag opened this issue May 25, 2020 · 4 comments
Closed

Support custom database label for sql.db tracing #231

cgstag opened this issue May 25, 2020 · 4 comments
Assignees

Comments

@cgstag
Copy link

cgstag commented May 25, 2020

Right now, when using xray.SQLContext(driver,DSN), in order to open a DB connection with tracing, the DSN is parsed and the dbname of the DSN is used to inject into the Capture function, used under the hood to trace the calls.
Exemple in the QueryContext method :

err = Capture(ctx, stmt.attr.dbname, func(ctx context.Context) error {
			stmt.populate(ctx)
			var err error
			result, err = stmt.Stmt.Query(dargs)
			return err
		})

However, in the case where two Databases hold the same dbname, but are different databases with different DSN (different host, different tables...) XRay will think it is the same database in the service map.

It would be really helpful to be able to set a custom label (that can default to attr.dbname).
I can contribute to make that feature happen, but I really dont know what is the best way to achieve this.

@cgstag cgstag changed the title Support custom span name / label for sql.db tracing Support custom database label for sql.db tracing May 25, 2020
@bhautikpip
Copy link
Contributor

Hi @cgstag ,

Thanks for opening this issue. We are looking into this issue to see what could be the best way to address this issue. Meanwhile if you have any ideas please post here so that we can also discuss that. Thank you for the patience!

@cgstag
Copy link
Author

cgstag commented Aug 25, 2020

Hello @bhautikpip , has this topic been discussed internally ?
I think the quickest / easiest way would be instead of using the dbname to just use the entire dsn or a combination of host + dbname. Another way is to provide another constructor with Options, and providing the display label as one of the options.

I tried to play around DSN parameters but not only is it dependant on the drivers used, it also can mess up the parsing altogether, so I think its a no go.

@bhautikpip
Copy link
Contributor

Hi @cgstag ,

I agree with you. dbname@host looks simpler and makes sense to me. I looked at the implementation of X-Ray Java SDK and
X-Ray .NET SDK. X-Ray Java SDK uses schema specific part as uri and .NET SDK uses dbname@datasourcename. I think we can go ahead with dbname@host for golang SDK. In the providing another constructor with Options approach customer would have to set that right? I believe if we can resolve this without adding any kind of configuration overhead on customer side then it would be nice. Feel free to post any other thoughts and submit a PR. I would be happy to review!

@bhautikpip bhautikpip mentioned this issue Jan 7, 2021
@bhautikpip
Copy link
Contributor

Hi @cgstag ,

X-Ray Go SDK v1.3.0 is released and includes this enhancement (#273) which probably solves naming issue when using 2 databases with same name but different hosts in the case of known DSL so closing this issue. Feel free to open new issue if you find any issues. :)

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

No branches or pull requests

2 participants