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

Switch to microsoft/mssqldb package #1569

Merged
merged 3 commits into from
Mar 12, 2024
Merged

Switch to microsoft/mssqldb package #1569

merged 3 commits into from
Mar 12, 2024

Conversation

dmathieu
Copy link
Member

The denisenkom/go-mssqldb explicitly says in its readme:

For more recent updates, see the Microsoft fork.

So this PR switches our use of that module for the official one from Microsoft.

@dmathieu dmathieu marked this pull request as ready for review January 23, 2024 11:01
@dmathieu dmathieu requested a review from a team as a code owner January 23, 2024 11:01
@@ -46,7 +46,7 @@ func DriverName(d driver.Driver) string {
return "postgresql"
}

if strings.HasPrefix(t.PkgPath(), "github.com/denisenkom/go-mssqldb") {
if strings.HasPrefix(t.PkgPath(), "github.com/microsoft/go-mssqldb") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should either support the old prefix or drop support for the old repo and release a new minor version (e.g. 2.5.0 ?)

Copy link
Member Author

@dmathieu dmathieu Jan 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Supporting the old prefix is tricky, as the only way we could do it is with two different driver names, which would be strange, as folks not using our instrumentation will not use the name we set here.
So I think releasing a minor version is what we need here.

@v1v
Copy link
Member

v1v commented Feb 8, 2024

run docs-build

@dmathieu dmathieu enabled auto-merge (squash) March 12, 2024 08:36
@dmathieu dmathieu merged commit 97d439f into main Mar 12, 2024
13 checks passed
@dmathieu dmathieu deleted the microsoft-mssqldb branch March 12, 2024 09:23
@thibleroy
Copy link

thibleroy commented Mar 15, 2024

Hello guys, thank you for the switch to microsoft package!
There might be an issue btw, I am not able to perform a SELECT Id FROM Table WHERE Id = $1 ; I get a no rows returned from row.Scan.
Actually I am able to retrieve rows using the formatted query:

// NOK
	query := "SELECT Id FROM Customer WHERE Id = $1"
	var customer Customer
	row := db.QueryRowContext(apm.ContextWithTransaction(context.Background(), tx), query, 2)
// OK
	query := fmt.Sprintf("SELECT Id FROM Customer WHERE Id = %d", 2)
	var customer Customer
	row := db.QueryRowContext(apm.ContextWithTransaction(context.Background(), tx), query)

Here is how I init my db instance:

package main

import (
	"context"
	"database/sql"
	"encoding/json"
	"fmt"
	"os"

	"github.com/sirupsen/logrus"
	"go.elastic.co/apm/module/apmsql/v2"
	_ "go.elastic.co/apm/module/apmsql/v2/sqlserver"
	"go.elastic.co/apm/v2"
)

func main() {
	settings, err := LoadSettings("server.conf.json")
	dsn := settings.Database.String("go-test")
	db, err := apmsql.Open("sqlserver", dsn)
	if err != nil {
		logrus.StandardLogger().Fatal(err)
	}
	defer db.Close()

@thibleroy
Copy link

We should update the documentation to add the sqlserver option in the driver list

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

Successfully merging this pull request may close these issues.

4 participants