Skip to content

Commit af0eedf

Browse files
crobert-1Fiery-Fenix
authored andcommitted
[receiver/sqlserver] Ensure all enabled metrics are emitted (open-telemetry#38839)
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description When directly connecting to SQL Server, this receiver runs multiple queries to get enabled metrics. Each query is only run if one or more metric is enabled for that particular query. The bug here was that some metrics weren't properly being checked to see if they were enabled. This means that in some cases, if a subset of metrics were enabled, the query wouldn't be run, and the metrics wouldn't be recorded or emitted. <!--Describe what testing was performed and which tests were added.--> #### Testing Added a test to ensure whenever new metrics are added, the `setupQueries` method gets changed as well. If we continue to hit more bugs with this we may want to just always run the queries, even if there's a slight performance hit.
1 parent 1fa1617 commit af0eedf

File tree

4 files changed

+86
-14
lines changed

4 files changed

+86
-14
lines changed
Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
# Use this changelog template to create an entry for release notes.
2+
3+
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
4+
change_type: bug_fix
5+
6+
# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver)
7+
component: receiver/sqlserver
8+
9+
# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
10+
note: Ensure all enabled metrics are emitted
11+
12+
# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists.
13+
issues: [38839]
14+
15+
# (Optional) One or more lines of additional information to render under the primary note.
16+
# These lines will be padded with 2 spaces and then inserted directly into the document.
17+
# Use pipe (|) for multiline entries.
18+
subtext:
19+
20+
# If your change doesn't affect end users or the exported elements of any package,
21+
# you should instead start your pull request title with [chore] or use the "Skip Changelog" label.
22+
# Optional: The change log or logs in which this entry should be included.
23+
# e.g. '[user]' or '[user, api]'
24+
# Include 'user' if the change is relevant to end users.
25+
# Include 'api' if there is a change to a library API.
26+
# Default: '[user]'
27+
change_logs: []

receiver/sqlserverreceiver/factory.go

Lines changed: 40 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -66,15 +66,7 @@ func setupQueries(cfg *Config) []string {
6666
queries = append(queries, getSQLServerDatabaseIOQuery(cfg.InstanceName))
6767
}
6868

69-
if cfg.MetricsBuilderConfig.Metrics.SqlserverBatchRequestRate.Enabled ||
70-
cfg.MetricsBuilderConfig.Metrics.SqlserverPageBufferCacheHitRatio.Enabled ||
71-
cfg.MetricsBuilderConfig.Metrics.SqlserverResourcePoolDiskThrottledReadRate.Enabled ||
72-
cfg.MetricsBuilderConfig.Metrics.SqlserverResourcePoolDiskThrottledWriteRate.Enabled ||
73-
cfg.MetricsBuilderConfig.Metrics.SqlserverLockWaitRate.Enabled ||
74-
cfg.MetricsBuilderConfig.Metrics.SqlserverProcessesBlocked.Enabled ||
75-
cfg.MetricsBuilderConfig.Metrics.SqlserverBatchSQLRecompilationRate.Enabled ||
76-
cfg.MetricsBuilderConfig.Metrics.SqlserverBatchSQLCompilationRate.Enabled ||
77-
cfg.MetricsBuilderConfig.Metrics.SqlserverUserConnectionCount.Enabled {
69+
if isPerfCounterQueryEnabled(&cfg.MetricsBuilderConfig.Metrics) {
7870
queries = append(queries, getSQLServerPerformanceCounterQuery(cfg.InstanceName))
7971
}
8072

@@ -256,10 +248,45 @@ func setupLogsScrapers(params receiver.Settings, cfg *Config) ([]scraperhelper.C
256248
}
257249

258250
func isDatabaseIOQueryEnabled(metrics *metadata.MetricsConfig) bool {
259-
if metrics.SqlserverDatabaseLatency.Enabled ||
251+
if metrics == nil {
252+
return false
253+
}
254+
255+
return metrics.SqlserverDatabaseLatency.Enabled ||
260256
metrics.SqlserverDatabaseOperations.Enabled ||
261-
metrics.SqlserverDatabaseIo.Enabled {
262-
return true
257+
metrics.SqlserverDatabaseIo.Enabled
258+
}
259+
260+
func isPerfCounterQueryEnabled(metrics *metadata.MetricsConfig) bool {
261+
if metrics == nil {
262+
return false
263263
}
264-
return false
264+
265+
return metrics.SqlserverBatchRequestRate.Enabled ||
266+
metrics.SqlserverBatchSQLCompilationRate.Enabled ||
267+
metrics.SqlserverBatchSQLRecompilationRate.Enabled ||
268+
metrics.SqlserverDatabaseBackupOrRestoreRate.Enabled ||
269+
metrics.SqlserverDatabaseExecutionErrors.Enabled ||
270+
metrics.SqlserverDatabaseFullScanRate.Enabled ||
271+
metrics.SqlserverDatabaseTempdbSpace.Enabled ||
272+
metrics.SqlserverDatabaseTempdbVersionStoreSize.Enabled ||
273+
metrics.SqlserverDeadlockRate.Enabled ||
274+
metrics.SqlserverIndexSearchRate.Enabled ||
275+
metrics.SqlserverLockTimeoutRate.Enabled ||
276+
metrics.SqlserverLockWaitRate.Enabled ||
277+
metrics.SqlserverLoginRate.Enabled ||
278+
metrics.SqlserverLogoutRate.Enabled ||
279+
metrics.SqlserverMemoryGrantsPendingCount.Enabled ||
280+
metrics.SqlserverMemoryUsage.Enabled ||
281+
metrics.SqlserverPageBufferCacheFreeListStallsRate.Enabled ||
282+
metrics.SqlserverPageBufferCacheHitRatio.Enabled ||
283+
metrics.SqlserverPageLookupRate.Enabled ||
284+
metrics.SqlserverProcessesBlocked.Enabled ||
285+
metrics.SqlserverReplicaDataRate.Enabled ||
286+
metrics.SqlserverResourcePoolDiskThrottledReadRate.Enabled ||
287+
metrics.SqlserverResourcePoolDiskThrottledWriteRate.Enabled ||
288+
metrics.SqlserverTableCount.Enabled ||
289+
metrics.SqlserverTransactionDelay.Enabled ||
290+
metrics.SqlserverTransactionMirrorWriteRate.Enabled ||
291+
metrics.SqlserverUserConnectionCount.Enabled
265292
}

receiver/sqlserverreceiver/factory_test.go

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ package sqlserverreceiver
55

66
import (
77
"context"
8+
"os"
89
"testing"
910
"time"
1011

@@ -15,6 +16,7 @@ import (
1516
"go.opentelemetry.io/collector/consumer/consumertest"
1617
"go.opentelemetry.io/collector/receiver/receivertest"
1718
"go.opentelemetry.io/collector/scraper/scraperhelper"
19+
"gopkg.in/yaml.v3"
1820

1921
"github.com/open-telemetry/opentelemetry-collector-contrib/receiver/sqlserverreceiver/internal/metadata"
2022
)
@@ -248,3 +250,19 @@ func TestNewCache(t *testing.T) {
248250
cache = newCache(0)
249251
require.NotNil(t, cache.Values())
250252
}
253+
254+
func TestSetupQueries(t *testing.T) {
255+
var metadata map[string]any
256+
257+
yamlFile, err := os.ReadFile("./metadata.yaml")
258+
require.NoError(t, err)
259+
require.NoError(t, yaml.Unmarshal(yamlFile, &metadata))
260+
require.NotNil(t, metadata["metrics"])
261+
262+
metricsMetadata, ok := metadata["metrics"].(map[string]any)
263+
require.True(t, ok)
264+
require.Len(t, metricsMetadata, 45,
265+
"Every time metrics are added or removed, the function `setupQueries` must "+
266+
"be modified to properly account for the change. Please update `setupQueries` and then, "+
267+
"and only then, update the expected metric count here.")
268+
}

receiver/sqlserverreceiver/go.mod

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ require (
2727
go.uber.org/goleak v1.3.0
2828
go.uber.org/multierr v1.11.0
2929
go.uber.org/zap v1.27.0
30+
gopkg.in/yaml.v3 v3.0.1
3031
)
3132

3233
require (
@@ -136,7 +137,6 @@ require (
136137
google.golang.org/genproto/googleapis/rpc v0.0.0-20250115164207-1a7da9e5054f // indirect
137138
google.golang.org/grpc v1.71.0 // indirect
138139
google.golang.org/protobuf v1.36.5 // indirect
139-
gopkg.in/yaml.v3 v3.0.1 // indirect
140140
)
141141

142142
replace github.com/open-telemetry/opentelemetry-collector-contrib/pkg/pdatatest => ../../pkg/pdatatest

0 commit comments

Comments
 (0)