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

Issue with DSN parsing when using with Databricks SQL Driver #358

Closed
nverkhachoyan opened this issue Sep 13, 2024 · 2 comments · Fixed by #359
Closed

Issue with DSN parsing when using with Databricks SQL Driver #358

nverkhachoyan opened this issue Sep 13, 2024 · 2 comments · Fixed by #359
Labels
bug Something isn't working

Comments

@nverkhachoyan
Copy link

Description

It looks like otelsql is passing an empty string as the DSN when calling sql.Open(driverName, ""). As a result, the Databricks SQL driver isn’t getting the proper DSN it needs to parse, which leads to the following error:

request error: invalid DSN: invalid DSN port: strconv.Atoi: parsing "": invalid syntax

The otelsql code that makes the function call

db, err := sql.Open(driverName, "")
if err != nil {
    return "", err
}

And the Databricks SQL Driver code that attempts to parse it.

func (d *databricksDriver) OpenConnector(dsn string) (driver.Connector, error) {
	ucfg, err := config.ParseDSN(dsn)
	if err != nil {
		return nil, err
	}
	return NewConnector(withUserConfig(ucfg))
}

Environment

  • Go Version: v1.22.5
  • otelsql version: v0.33.0
  • Databricks SQL Go v1.6.1

Steps To Reproduce

The simple code snippet below is enough to reproduce the error.

package main

import (
	"fmt"
	"os"

	"github.com/XSAM/otelsql"
	_ "github.com/databricks/databricks-sql-go"
	"go.opentelemetry.io/otel/attribute"
)

func main() {
	dsn := os.Getenv("DATABRICKS_DSN")

	db, err := otelsql.Open("databricks", dsn, otelsql.WithAttributes(
		attribute.String("db.system", "databricks"),
	), otelsql.WithSpanOptions(otelsql.SpanOptions{
		DisableQuery: true,
	}))
	if err != nil {
		fmt.Printf("error database connection: %v\n", err)
		return
	}

	var id int
	err = db.QueryRow("SELECT 1").Scan(&id)
	if err != nil {
		fmt.Printf("failed to fetch database: %v\n", err)
		return
	}

	fmt.Printf("Database responded: %v\n", id)
}

Expected behavior

The Databricks driver should properly receive and parse the DSN string, and the opened connection should be wrapped with otelsql.

Actual behavior

Returns an invalid DSN error

request error: invalid DSN: invalid DSN port: strconv.Atoi: parsing "": invalid syntax
@nverkhachoyan nverkhachoyan added the bug Something isn't working label Sep 13, 2024
@XSAM
Copy link
Owner

XSAM commented Sep 13, 2024

Hi @nverkhachoyan, thanks for raising this issue.

otelsql does not wrap the sql.DB (at least for the current implementation). So otelsql.Open uses sql.Open to only get the driver.

otelsql/sql.go

Lines 78 to 87 in f079f75

func Open(driverName, dataSourceName string, options ...Option) (*sql.DB, error) {
// Retrieve the driver implementation we need to wrap with instrumentation
db, err := sql.Open(driverName, "")
if err != nil {
return nil, err
}
d := db.Driver()
if err = db.Close(); err != nil {
return nil, err
}

There is a potential way that I can try to pass the DSN to the sql.Open even though otelsql does not use it to actually open a connection. But, I need to run some tests to verify this.

Before that, you can use otelsq.WrapDriver method to bypass this issue. Example: https://pkg.go.dev/github.com/XSAM/otelsql#example-WrapDriver

db, err := sql.Open("databricks", dsn)

otDriver := otelsql.WrapDriver(dri)

connector, err := otDriver.(driver.DriverContext).OpenConnector(mysqlDSN)
if err != nil {
	panic(err)
}

// Connect to database
db = sql.OpenDB(connector)
defer db.Close()

@XSAM
Copy link
Owner

XSAM commented Sep 15, 2024

@nverkhachoyan I have made a release that includes the fix. https://github.com/XSAM/otelsql/releases/tag/v0.34.0

You can now use otelsql.Open with Databricks SQL Driver.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants