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

swap sqlserver drivers: denisenkom/go-mssqldb -> microsoft/go-mssqldb #27200

Closed
cwegener opened this issue Sep 26, 2023 · 7 comments
Closed

swap sqlserver drivers: denisenkom/go-mssqldb -> microsoft/go-mssqldb #27200

cwegener opened this issue Sep 26, 2023 · 7 comments
Labels
good first issue Good for newcomers help wanted Extra attention is needed receiver/sqlquery SQL query receiver

Comments

@cwegener
Copy link
Contributor

Component(s)

receiver/sqlquery

Describe the issue you're reporting

Round about the same time as the sqlqueryreceiver was created in 2022, Microsoft has created an official fork of https://github.com/denisenkom/go-mssqldb at https://github.com/microsoft/go-mssqldb in agreement with the original project maintainers denisenkom/go-mssqldb#729

Since then, there have been plenty of fixes and enhancement delivered under the new Microsoft umbrella and 3 official versioned releases.

And the original project is receiving the minimum maintenance effort.

From the above it seems to me that the otel collector should adopt the new fork of go-mssqldb in the sqlqueryreceiver.

@cwegener cwegener added the needs triage New item requiring triage label Sep 26, 2023
@github-actions github-actions bot added the receiver/sqlquery SQL query receiver label Sep 26, 2023
@github-actions
Copy link
Contributor

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@dmitryax dmitryax added help wanted Extra attention is needed good first issue Good for newcomers and removed needs triage New item requiring triage labels Sep 26, 2023
@dmitryax
Copy link
Member

Sounds reasonable to me. @cwegener do you have a chance to help with this?

Copy link
Contributor

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

  • issue: Github issue template generation code needs this to generate the corresponding labels.
  • receiver/sqlquery: @dmitryax @pmcollins

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@github-actions github-actions bot added the Stale label Nov 27, 2023
@cwegener
Copy link
Contributor Author

Right. Looks like I better jump through all the hoops to get this out ...

Questions:

  1. Would this change be considered a 'behaviour change' in the eyes of the Contributing doc? https://github.com/open-telemetry/opentelemetry-collector/blob/main/CONTRIBUTING.md#refactoring-work
--- a/upstream/receiver/sqlqueryreceiver/db_client.go
+++ b/upstream/receiver/sqlqueryreceiver/db_client.go
@@ -8,9 +8,10 @@ import (

        // register db drivers
        _ "github.com/SAP/go-hdb/driver"
-       _ "github.com/denisenkom/go-mssqldb"
        _ "github.com/go-sql-driver/mysql"
        _ "github.com/lib/pq"
+       _ "github.com/microsoft/go-mssqldb"
+       _ "github.com/microsoft/go-mssqldb/integratedauth/krb5"
        _ "github.com/sijms/go-ora/v2"
        _ "github.com/snowflakedb/gosnowflake"
        "go.uber.org/multierr"

I think it might be possible to split out the krb5 addition into a separate change, but then the swap from the old to the new driver could be considered broken ... I'll have to test out if the new MS driver works without the krb5 module ....

@github-actions github-actions bot removed the Stale label Nov 28, 2023
@andrzej-stencel
Copy link
Member

Would this change be considered a 'behaviour change' in the eyes of the Contributing doc? https://github.com/open-telemetry/opentelemetry-collector/blob/main/CONTRIBUTING.md#refactoring-work

I wouldn't sweat over it and would just add this new dependency "github.com/microsoft/go-mssqldb/integratedauth/krb5" if there's a chance that not adding it could break existing functionality.

@cwegener
Copy link
Contributor Author

cwegener commented Dec 6, 2023

I'll just need to write up a CHANGELOG entry and then I'm good to go I think. I'll send through the patch via GH PR soon.

cwegener added a commit to cwegener/opentelemetry-collector-contrib that referenced this issue Jan 3, 2024
**Description:**

The old driver ('denisenkom') is only receiving limited attention. The
new driver ('microsoft') is maintained by the vendor of the actual RDBMS
now. And is also the driver listed on golang.org

**Link to tracking Issue:**

open-telemetry#27200

**Testing:**

No integration tests exist for MSSQL, only for Oracle, Postgres and MySQL

TODO: add MSSQL integration tests.

**Documentation:**

Revelant documentation is available upstream:
https://github.com/microsoft/go-mssqldb/blob/main/README.md

make gotidy

Signed-off-by: Christoph Wegener <cwegener@users.noreply.github.com>
dmitryax pushed a commit that referenced this issue Jan 3, 2024
**Description:**

The old driver ('denisenkom') is only receiving limited attention. The
new driver ('microsoft') is maintained by the vendor of the actual RDBMS
now. And is also the driver listed on golang.org

**Link to tracking Issue:**

#27200

**Documentation:**

Revelant documentation is available upstream:
https://github.com/microsoft/go-mssqldb/blob/main/README.md

Signed-off-by: Christoph Wegener <cwegener@users.noreply.github.com>
cparkins pushed a commit to AmadeusITGroup/opentelemetry-collector-contrib that referenced this issue Jan 10, 2024
…emetry#29694)

**Description:**

The old driver ('denisenkom') is only receiving limited attention. The
new driver ('microsoft') is maintained by the vendor of the actual RDBMS
now. And is also the driver listed on golang.org

**Link to tracking Issue:**

open-telemetry#27200

**Documentation:**

Revelant documentation is available upstream:
https://github.com/microsoft/go-mssqldb/blob/main/README.md

Signed-off-by: Christoph Wegener <cwegener@users.noreply.github.com>
@andrzej-stencel
Copy link
Member

Resolved in #29694

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed receiver/sqlquery SQL query receiver
Projects
None yet
Development

No branches or pull requests

3 participants