Enable direct connection to SQL Server instance #30297

crobert-1 · 2024-01-04T19:45:23Z

Component(s)

receiver/sqlserver

Is your feature request related to a problem? Please describe.

The existing implementation of the SQL Server receiver only collects information from Windows Performance counters. This was by design with the introduction of the receiver, as this solution provided good value for the amount of effort required (source). The main limitation was noted in the proposal as there would be metrics not covered by this solution, but it was acceptable at the time (source).

At this time there are two main drawbacks with the receiver's functionality:

As noted, there are useful metrics that can not be gathered by this solution. I've received customer requests for some of these recently, as described in [receiver/sqlserver] Request to add support for more metrics #29865.
SQL Server can be installed in Windows, Linux, and Docker environments, but the receiver can only be run in Windows.

Describe the solution you'd like

I want the sqlserver receiver to be able to run in all environments supported by SQL server itself, as well as scraping more metrics that can help users understand the status of the SQL server instances.

My proposed solution would be to add the sqlquery receiver as an internal dependency of the sqlserver receiver. The sqlserver receiver could expose metrics to the user as it does now, but internally rely on the sqlquery receiver to send the queries that would give metrics we want. This would require the addition of more configuration options for connection to the DB, as well as defining the metrics and queries we want.

Adding a receiver as a dependency of another may not be the best design, so a possible answer to this may also involve moving some sqlquery receiver logic into a central package as suggested here. This would allow re-using the same logic in multiple places without duplicating code.

Describe alternatives you've considered

Require users to manually provide all relevant queries, and use the sqlquery receiver directly. This is a poor UX experience and very liable to misconfigurations. It also doesn't provide metric naming semantics, or re-usability for different users.
Directly rely on the Telegraf SQL Server Plugin Input. This would change the receiver's configuration, as more options would be required to interface with Telegraf and to directly connect to a running SQL Server instance.There has been some discussion historically around directly using Telegraf plugins. This has been suggested in core, and here's an article of a working (at the time) implementation.This would allow us to leverage existing connection logic, as well as existing queries. Potential drawbacks may be relying on the Telegraf plugin itself, or potentially performance concerns around converting metrics received from telegraf into Otel's format. This would also be a bit more complex to implement.
Rely on Telegraf's OpenTelemetry output plugin to send data to the collector, then use the influxdb receiver to ingest it. This works but is definitely a more complicated running environment for users.

Additional context

No response

github-actions · 2024-01-04T19:45:42Z

Pinging code owners:

receiver/sqlserver: @djaglowski @StefanKurek

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

github-actions · 2024-01-04T19:59:04Z

Pinging code owners for receiver/sqlquery: @dmitryax @pmcollins. See Adding Labels via Comments if you do not have permissions to add labels yourself.

djaglowski · 2024-01-09T15:39:17Z

This makes sense to me. I would prefer that we not depend directly on another receiver though, and instead take the approach where we move common code to a shared package.

…30709) **Description:** As noted in the related issues, a lot of the logic in the sqlquery receiver can be moved to a central package to be used by other receivers. I realize this appears to be a large change, but it's purely a re-organization PR, there's no functional changes. GitHub is not recognizing moved files, so please refer to new file names and their deleted counterparts. A lot of members are now public in the internal package that were private in the receiver which is likely what's causing GitHub to miss that they're just moved files. This doesn't require a changelog (in my mind) because all things moving were originally private to the receiver. They're now still purely internal, so they won't be published. I believe `internal` is the best destination for now as it allows us to solidify the usage and interface before moving it to `pkg` to be publicly available. There are still a lot of unknowns as far as how it can be used by other receivers, so I fully expect "breaking changes" to this. If we keep it internal we can update all usages with the changing interface, so the changes won't be breaking. **Link to tracking Issue:** #30297, #13546

…pen-telemetry#30709) **Description:** As noted in the related issues, a lot of the logic in the sqlquery receiver can be moved to a central package to be used by other receivers. I realize this appears to be a large change, but it's purely a re-organization PR, there's no functional changes. GitHub is not recognizing moved files, so please refer to new file names and their deleted counterparts. A lot of members are now public in the internal package that were private in the receiver which is likely what's causing GitHub to miss that they're just moved files. This doesn't require a changelog (in my mind) because all things moving were originally private to the receiver. They're now still purely internal, so they won't be published. I believe `internal` is the best destination for now as it allows us to solidify the usage and interface before moving it to `pkg` to be publicly available. There are still a lot of unknowns as far as how it can be used by other receivers, so I fully expect "breaking changes" to this. If we keep it internal we can update all usages with the changing interface, so the changes won't be breaking. **Link to tracking Issue:** open-telemetry#30297, open-telemetry#13546

github-actions · 2024-03-12T03:29:52Z

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:

receiver/sqlquery: @dmitryax @crobert-1
receiver/sqlserver: @djaglowski @StefanKurek

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

**Description:** <Describe what has changed.>  Add a query for the database IO metrics that SQL Server exposes. Read more about available metrics [here](https://learn.microsoft.com/en-us/sql/relational-databases/system-dynamic-management-views/sys-dm-io-virtual-file-stats-transact-sql?view=sql-server-ver16). This is a no-op update as this query will not be used until direct connection to the SQL Server instance is fully implemented. This is simply part of the effort. **Note:** This code is currently not reached. This is on purpose. **Link to tracking Issue:** <Issue number if applicable> This was originally part of #31915, but I'm breaking this out to make the original PR a more manageable size. #30297 **Testing:** <Describe what testing was performed and which tests were added.> Existing tests and added tests are passing. **Documentation:** <Describe the documentation added.> Purposefully none: This is currently dead code until all of #31915 gets merged, so it shouldn't be used.

…etry#32178) **Description:** <Describe what has changed.>  Add a query for the database IO metrics that SQL Server exposes. Read more about available metrics [here](https://learn.microsoft.com/en-us/sql/relational-databases/system-dynamic-management-views/sys-dm-io-virtual-file-stats-transact-sql?view=sql-server-ver16). This is a no-op update as this query will not be used until direct connection to the SQL Server instance is fully implemented. This is simply part of the effort. **Note:** This code is currently not reached. This is on purpose. **Link to tracking Issue:** <Issue number if applicable> This was originally part of open-telemetry#31915, but I'm breaking this out to make the original PR a more manageable size. open-telemetry#30297 **Testing:** <Describe what testing was performed and which tests were added.> Existing tests and added tests are passing. **Documentation:** <Describe the documentation added.> Purposefully none: This is currently dead code until all of open-telemetry#31915 gets merged, so it shouldn't be used.

) **Description:** <Describe what has changed.>  This adds config options for direct connection to a SQL server instance. For the sake of PR size, this adds no functionality, it's simply config options and validation. **Link to tracking Issue:** <Issue number if applicable> This was originally part of #31915, but I'm breaking this out to make the original PR a more manageable size. #30297 **Testing:** <Describe what testing was performed and which tests were added.> Existing tests and added tests are all passing. **Documentation:** <Describe the documentation added.> No documentation on purpose. The added config options currently have no impact on functionality, so users shouldn't be aware of this, and shouldn't try to use them yet.

…32471) **Description:** <Describe what has changed.>  A major part of #31915 is adding support for other operating systems than Windows. This change moves Windows-specific logic into files named as such, and renames some specific functionality to match purpose, instead of generic names. `newSQLServerScraper` -> `newSQLServerPCScraper` (PC in this context stands for performance counters) `sqlServerScraper` -> `sqlServerPCScraper`` This only renames internal functions and files, there's no user-facing impact of this. **Link to tracking Issue:** <Issue number if applicable> #31915 #30297 **Testing:** <Describe what testing was performed and which tests were added.> Existing tests are passing.

…tems (#32542) **Description:** <Describe what has changed.>  This change enables the SQL Server receiver to directly connect to SQL Server instances, and to run on operating systems other than Windows. This is part of #31915, but doesn't add any functionality for the end user. No metrics are currently being gathered from SQL server instances, to make the PR (hopefully) simpler to review. **Link to tracking Issue:** <Issue number if applicable> #31915 #30297 **Testing:** <Describe what testing was performed and which tests were added.> Tests added are passing.

…#31915) **Description:** <Describe what has changed.>  This PR enables direct connection to SQL Server instances, and also allows the SQL Server receiver to run on non-Windows platforms. User-facing changes: 1. More (optional) configuration options. If the receiver wants to connect directly to the SQL Server instance (rather than relying on Windows Performance Counters), all of these options must be specified. 2. Add a new metric, `sqlserver.database_io.read_latency`. This is disabled by default, and way randomly chosen as a single metric to try to restrict this PR's size. I plan on following this PR up with more metrics from the same query, as well as more queries and metrics. Note: Existing users should see no change in functionality with this PR. Using existing configurations with this PR should have no performance or functional impact. **Link to tracking Issue:** <Issue number if applicable> Resolves #30297 **Testing:** <Describe what testing was performed and which tests were added.> Tested on MacOS and Windows. Manual tested with direct connection to a docker container on both platforms, as well as ran all tests in package. Everything is passing. **Working demo for testing:** Docker container start command: ``` docker run -e "ACCEPT_EULA=Y" -e "MSSQL_SA_PASSWORD=StrongPassw0rd" -p 1433:1433 -d mcr.microsoft.com/mssql/server:2022-latest ``` Collector config: ``` sqlserver: password: "StrongPassw0rd" port: 1433 server: 0.0.0.0 username: sa resource_attributes: sqlserver.instance.name: enabled: true sqlserver.computer.name: enabled: true metrics: sqlserver.database_io.read_latency: enabled: true ``` **Documentation:** <Describe the documentation added.> Updated README

… OS (#32878) **Description:** <Describe what has changed.>  With the enhancement in #30297, the SQL Server receiver can now run on MacOS and Linux, since it no longer solely relies on Windows. The difference in functionality has (hopefully) been documented clearly between the different platforms, so the unsupported warning can be removed. I missed this in my previous PRs.

…etry#32178) **Description:** <Describe what has changed.>  Add a query for the database IO metrics that SQL Server exposes. Read more about available metrics [here](https://learn.microsoft.com/en-us/sql/relational-databases/system-dynamic-management-views/sys-dm-io-virtual-file-stats-transact-sql?view=sql-server-ver16). This is a no-op update as this query will not be used until direct connection to the SQL Server instance is fully implemented. This is simply part of the effort. **Note:** This code is currently not reached. This is on purpose. **Link to tracking Issue:** <Issue number if applicable> This was originally part of open-telemetry#31915, but I'm breaking this out to make the original PR a more manageable size. open-telemetry#30297 **Testing:** <Describe what testing was performed and which tests were added.> Existing tests and added tests are passing. **Documentation:** <Describe the documentation added.> Purposefully none: This is currently dead code until all of open-telemetry#31915 gets merged, so it shouldn't be used.

…n-telemetry#32176) **Description:** <Describe what has changed.>  This adds config options for direct connection to a SQL server instance. For the sake of PR size, this adds no functionality, it's simply config options and validation. **Link to tracking Issue:** <Issue number if applicable> This was originally part of open-telemetry#31915, but I'm breaking this out to make the original PR a more manageable size. open-telemetry#30297 **Testing:** <Describe what testing was performed and which tests were added.> Existing tests and added tests are all passing. **Documentation:** <Describe the documentation added.> No documentation on purpose. The added config options currently have no impact on functionality, so users shouldn't be aware of this, and shouldn't try to use them yet.

…pen-telemetry#32471) **Description:** <Describe what has changed.>  A major part of open-telemetry#31915 is adding support for other operating systems than Windows. This change moves Windows-specific logic into files named as such, and renames some specific functionality to match purpose, instead of generic names. `newSQLServerScraper` -> `newSQLServerPCScraper` (PC in this context stands for performance counters) `sqlServerScraper` -> `sqlServerPCScraper`` This only renames internal functions and files, there's no user-facing impact of this. **Link to tracking Issue:** <Issue number if applicable> open-telemetry#31915 open-telemetry#30297 **Testing:** <Describe what testing was performed and which tests were added.> Existing tests are passing.

…tems (open-telemetry#32542) **Description:** <Describe what has changed.>  This change enables the SQL Server receiver to directly connect to SQL Server instances, and to run on operating systems other than Windows. This is part of open-telemetry#31915, but doesn't add any functionality for the end user. No metrics are currently being gathered from SQL server instances, to make the PR (hopefully) simpler to review. **Link to tracking Issue:** <Issue number if applicable> open-telemetry#31915 open-telemetry#30297 **Testing:** <Describe what testing was performed and which tests were added.> Tests added are passing.

…open-telemetry#31915) **Description:** <Describe what has changed.>  This PR enables direct connection to SQL Server instances, and also allows the SQL Server receiver to run on non-Windows platforms. User-facing changes: 1. More (optional) configuration options. If the receiver wants to connect directly to the SQL Server instance (rather than relying on Windows Performance Counters), all of these options must be specified. 2. Add a new metric, `sqlserver.database_io.read_latency`. This is disabled by default, and way randomly chosen as a single metric to try to restrict this PR's size. I plan on following this PR up with more metrics from the same query, as well as more queries and metrics. Note: Existing users should see no change in functionality with this PR. Using existing configurations with this PR should have no performance or functional impact. **Link to tracking Issue:** <Issue number if applicable> Resolves open-telemetry#30297 **Testing:** <Describe what testing was performed and which tests were added.> Tested on MacOS and Windows. Manual tested with direct connection to a docker container on both platforms, as well as ran all tests in package. Everything is passing. **Working demo for testing:** Docker container start command: ``` docker run -e "ACCEPT_EULA=Y" -e "MSSQL_SA_PASSWORD=StrongPassw0rd" -p 1433:1433 -d mcr.microsoft.com/mssql/server:2022-latest ``` Collector config: ``` sqlserver: password: "StrongPassw0rd" port: 1433 server: 0.0.0.0 username: sa resource_attributes: sqlserver.instance.name: enabled: true sqlserver.computer.name: enabled: true metrics: sqlserver.database_io.read_latency: enabled: true ``` **Documentation:** <Describe the documentation added.> Updated README

… OS (open-telemetry#32878) **Description:** <Describe what has changed.>  With the enhancement in open-telemetry#30297, the SQL Server receiver can now run on MacOS and Linux, since it no longer solely relies on Windows. The difference in functionality has (hopefully) been documented clearly between the different platforms, so the unsupported warning can be removed. I missed this in my previous PRs.

…open-telemetry#31915) **Description:** <Describe what has changed.>  This PR enables direct connection to SQL Server instances, and also allows the SQL Server receiver to run on non-Windows platforms. User-facing changes: 1. More (optional) configuration options. If the receiver wants to connect directly to the SQL Server instance (rather than relying on Windows Performance Counters), all of these options must be specified. 2. Add a new metric, `sqlserver.database_io.read_latency`. This is disabled by default, and way randomly chosen as a single metric to try to restrict this PR's size. I plan on following this PR up with more metrics from the same query, as well as more queries and metrics. Note: Existing users should see no change in functionality with this PR. Using existing configurations with this PR should have no performance or functional impact. **Link to tracking Issue:** <Issue number if applicable> Resolves open-telemetry#30297 **Testing:** <Describe what testing was performed and which tests were added.> Tested on MacOS and Windows. Manual tested with direct connection to a docker container on both platforms, as well as ran all tests in package. Everything is passing. **Working demo for testing:** Docker container start command: ``` docker run -e "ACCEPT_EULA=Y" -e "MSSQL_SA_PASSWORD=StrongPassw0rd" -p 1433:1433 -d mcr.microsoft.com/mssql/server:2022-latest ``` Collector config: ``` sqlserver: password: "StrongPassw0rd" port: 1433 server: 0.0.0.0 username: sa resource_attributes: sqlserver.instance.name: enabled: true sqlserver.computer.name: enabled: true metrics: sqlserver.database_io.read_latency: enabled: true ``` **Documentation:** <Describe the documentation added.> Updated README

… OS (open-telemetry#32878) **Description:** <Describe what has changed.>  With the enhancement in open-telemetry#30297, the SQL Server receiver can now run on MacOS and Linux, since it no longer solely relies on Windows. The difference in functionality has (hopefully) been documented clearly between the different platforms, so the unsupported warning can be removed. I missed this in my previous PRs.

crobert-1 added enhancement New feature or request needs triage New item requiring triage labels Jan 4, 2024

github-actions bot added the receiver/sqlserver label Jan 4, 2024

crobert-1 added the receiver/sqlquery SQL query receiver label Jan 4, 2024

github-actions bot mentioned this issue Jan 9, 2024

Weekly Report: 2024-01-02 - 2024-01-09 #30334

Closed

djaglowski removed the needs triage New item requiring triage label Jan 9, 2024

crobert-1 self-assigned this Jan 11, 2024

This was referenced Jan 19, 2024

[chore][receiver/sqlquery] Move reusable logic to internal package #30689

Closed

[chore][receiver/sqlquery] Move reusable logic to internal package #30709

Merged

github-actions bot added the Stale label Mar 12, 2024

crobert-1 removed the Stale label Mar 12, 2024

crobert-1 mentioned this issue Mar 22, 2024

[receiver/sqlserver] Enable direct connection to SQL Server instances #31915

Merged

This was referenced Apr 4, 2024

[chore][receiver/sqlserver] Add direct connection config options #32176

Merged

[chore][receiver/sqlserver] Add database IO metrics query #32178

Merged

crobert-1 mentioned this issue Apr 16, 2024

[chore][receiver/sqlserver] Rename and move Windows-specific logic #32471

Merged

crobert-1 mentioned this issue Apr 18, 2024

[chore][receiver/sqlserver] Enable receiver to run on non-Windows systems #32542

Merged

djaglowski closed this as completed in #31915 Apr 23, 2024

crobert-1 mentioned this issue May 6, 2024

[chore][receiver/sqlserver] Update documentation for darwin and linux OS #32878

Merged

TigerC10 mentioned this issue May 10, 2024

Add support for Windows Authentication for direct connection to SQL Server instance #32986

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable direct connection to SQL Server instance #30297

Enable direct connection to SQL Server instance #30297

crobert-1 commented Jan 4, 2024 •

edited

github-actions bot commented Jan 4, 2024

github-actions bot commented Jan 4, 2024

djaglowski commented Jan 9, 2024

github-actions bot commented Mar 12, 2024

Enable direct connection to SQL Server instance #30297

Enable direct connection to SQL Server instance #30297

Comments

crobert-1 commented Jan 4, 2024 • edited

Component(s)

Is your feature request related to a problem? Please describe.

Describe the solution you'd like

Describe alternatives you've considered

Additional context

github-actions bot commented Jan 4, 2024

github-actions bot commented Jan 4, 2024

djaglowski commented Jan 9, 2024

github-actions bot commented Mar 12, 2024

crobert-1 commented Jan 4, 2024 •

edited