Support gormio

[Deepak13245]

Resolves #810

[cla-checker-service]

💚 CLA has been signed

[apmmachine]

💚 Build Succeeded

Expand to view the summary

Build stats

• Build Cause: [Pull request #825 updated]

• Start Time: 2020-10-07T06:46:44.082+0000

• Duration: 26 min 50 sec

Test stats 🧪

Test	Results
Failed	0
Passed	7571
Skipped	182
Total	7753

[axw]

@Deepak13245 thanks for the PR!

Looks pretty good, but I'm wondering if the callbacks are still necessary. If context is now first class, then perhaps we can instead just rely on apmsql to create the spans?

Also, I'd like to call this module apmgormv2 to be more in line with other modules.

[Deepak13245]

@axw renamed to apmgormv2
regarding the creation of spans by apmsql, gorm doesn't accept *sql.DB directly in params. There is a workaround so I raised a separate PR Deepak13245#1. Please suggest if there is a better approach.

Thanks

[axw]

A couple of things bother me about the plugin approach:

• It closes and reopens the connection. Not the end of the world, just a bit annoying.
• As it is at the moment, extractDsn forces a dependency on all the drivers. We could solve this by having the "_" dialect imports register a function though.

What do you think about providing "Open" functions inside the dialect packages instead? So instead of

import (
    "gorm.io/driver/sqlite"
    "gorm.io/gorm"         
)

db, err := gorm.Open(sqlite.Open("gorm.db"), &gorm.Config{})

you could write

import (
    "go.elastic.co/apm/module/apmgormv2/dialect/apmsqlite"
    "gorm.io/gorm"
)

db, err := gorm.Open(apmsqlite.Open("gorm.db"), &gorm.Config{})

Internally this would use the recently introduced support for custom drivers: go-gorm/sqlite#11

Basically just:

package apmsqlite // go.elastic.co/module/apmgormv2/dialect/apmsqlite

import (
    "gorm.io/driver/sqlite"
    "gorm.io/gorm"
    "go.elastic.co/module/apm/module/apmsql"
    _ "go.elastic.co/module/apm/module/apmsql/sqlite3"
)

func Open(dsn string) gorm.Dialector {
        return &sqlite.Dialector{
                DriverName: apmsql.DriverPrefix + sqlite.DriverName,
                DSN:        dsn,
        }
}

[Deepak13245]

@axw creating dialect from with driver prefix makes it neat, updated the pr please check.

[axw]

Thanks! I agree this looks much neater.

[axw]

jenkins run the tests please

[axw]

Perfect, thanks!

[axw]

jenkins run the tests please

[axw]

[2020-10-07T02:40:59.696Z] ../../gorm.io/gorm/utils/utils.go:46:30: reflect.ValueOf(val).IsZero undefined (type reflect.Value has no field or method IsZero)

It looks like gorm v2 requires Go 1.13+, so we'll need to update the build constraint.

[axw]

Looks good, I think the remaining test failures are fixed on master. Can you please merge master so we can make sure?

[axw]

CI looks happy. Thanks for another nice PR, and for bearing with my requests :)