aws / aws-xray-sdk-go

AWS X-Ray SDK for the Go programming language.
Apache License 2.0
278 stars 117 forks source link

Support custom database label for sql.db tracing #231

Closed cgstag closed 3 years ago

cgstag commented 4 years ago

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.

bhautikpip commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

Hi @cgstag ,

I agree with you. dbname@hostlooks 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 commented 3 years ago

Hi @cgstag ,

X-Ray Go SDK v1.3.0 is released and includes this enhancement (https://github.com/aws/aws-xray-sdk-go/pull/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. :)