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

[receiver/sqlserver] Properly scrape DB when metrics are enabled #34065

Merged
merged 2 commits into from
Jul 15, 2024

Conversation

crobert-1
Copy link
Member

@crobert-1 crobert-1 commented Jul 12, 2024

Description:

The bug here is in the setupQueries method. The goal is to only scrape a SQL Server instance if metrics gathered from that scrape are enabled. However, for the performance counter query, I found that some metrics weren't properly checked. This means that if sqlserver.lock.wait.rate is disabled, none of the other metrics will be scraped, even if they're enabled. This resolves the issue so that all metrics being gathered from this query can trigger this scrape.

To confirm all required metrics are checked here, view this method to see which metrics are being scraped using the perf counter query.

Testing:
Updated tests, double checked manually to ensure all metrics are properly being scraped.

Documentation:

@@ -43,10 +43,15 @@ func setupQueries(cfg *Config) []string {
queries = append(queries, getSQLServerDatabaseIOQuery(cfg.InstanceName))
}

if cfg.MetricsBuilderConfig.Metrics.SqlserverResourcePoolDiskThrottledReadRate.Enabled ||
Copy link
Member Author

@crobert-1 crobert-1 Jul 12, 2024

Choose a reason for hiding this comment

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

This issue could pretty easily happen again. It may be wise to change this functionality to always scrape and attempt to record the metrics, even if none are enabled. This would be some level of performance hit in some situations, but reduce complexity and reduce risk of potentially hard to find bugs in the future.

@crobert-1
Copy link
Member Author

Failing tests are frequencies of #34042 and #33998

@djaglowski djaglowski merged commit a16cc64 into open-telemetry:main Jul 15, 2024
154 checks passed
@github-actions github-actions bot added this to the next release milestone Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants