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.

Sign up for GitHub

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

Jump to bottom

[receiver/oracledb] Fix incorrect values for a couple of metrics #32028

Merged
merged 2 commits into from
Mar 28, 2024
Merged

[receiver/oracledb] Fix incorrect values for a couple of metrics #32028

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
92ee470
[recevier/oracledb] Fix incorrect values for a couple metrics
crobert-1 Mar 28, 2024
6c0f031
Use inner join on query as requested.
crobert-1 Mar 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions .chloggen/oracledb_tablespace_fix.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
# Use this changelog template to create an entry for release notes.

# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: bug_fix

# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver)
component: oracledbreceiver

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Fix incorrect values being set for oracledb.tablespace_size.limit and oracledb.tablespace_size.usage

# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists.
issues: [31451]

# (Optional) One or more lines of additional information to render under the primary note.
# These lines will be padded with 2 spaces and then inserted directly into the document.
# Use pipe (|) for multiline entries.
subtext:

# If your change doesn't affect end users or the exported elements of any package,
# you should instead start your pull request title with [chore] or use the "Skip Changelog" label.
# Optional: The change log or logs in which this entry should be included.
# e.g. '[user]' or '[user, api]'
# Include 'user' if the change is relevant to end users.
# Include 'api' if there is a change to a library API.
# Default: '[user]'
change_logs: []
7 changes: 1 addition & 6 deletions receiver/oracledbreceiver/internal/metadata/generated_metrics.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion receiver/oracledbreceiver/internal/metadata/generated_metrics_test.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion receiver/oracledbreceiver/metadata.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,6 @@ metrics:
enabled: true
gauge:
value_type: int
input_type: string
Copy link
Member Author

@crobert-1 crobert-1 Mar 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The logic for setting this metric's value now requires two DB values to be multiplied by each other. This means we have to manually convert both values to int64, multiply them, then use that result to set the value here. Converting back to string just to convert to int64 again is unnecessary, so I've removed this option.

unit: By
oracledb.db_block_gets:
description: Number of times a current block was requested from the buffer cache.
Expand Down
58 changes: 33 additions & 25 deletions receiver/oracledbreceiver/scraper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,10 @@ const (
consistentGets = "consistent gets"
sessionCountSQL = "select status, type, count(*) as VALUE FROM v$session GROUP BY status, type"
systemResourceLimitsSQL = "select RESOURCE_NAME, CURRENT_UTILIZATION, LIMIT_VALUE, CASE WHEN TRIM(INITIAL_ALLOCATION) LIKE 'UNLIMITED' THEN '-1' ELSE TRIM(INITIAL_ALLOCATION) END as INITIAL_ALLOCATION, CASE WHEN TRIM(LIMIT_VALUE) LIKE 'UNLIMITED' THEN '-1' ELSE TRIM(LIMIT_VALUE) END as LIMIT_VALUE from v$resource_limit"
tablespaceUsageSQL = "select TABLESPACE_NAME, BYTES from DBA_DATA_FILES"
tablespaceMaxSpaceSQL = "select TABLESPACE_NAME, (BLOCK_SIZE*MAX_EXTENTS) AS VALUE FROM DBA_TABLESPACES"
tablespaceUsageSQL = `
Copy link
Member Author

Choose a reason for hiding this comment

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

Feedback on indentation here would be helpful, not sure what's the most clear here.

Copy link
Contributor

Choose a reason for hiding this comment

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

It was readable enough. I had a bit of hesitation that the query itself is 2 separate subqueries, joined via a where statement with a bigger query. It feels like an inner join could do the job, but my SQL is rusty and this is an improvement over the broken functionality. Maybe something to explore if it becomes a performance flare up, which I doubt.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair point. I added a benchmark test locally to compare this query vs. inner join, here are the results.
Inner join:

BenchmarkSQLUsageMetrics-16    	     150	   6881141 ns/op
BenchmarkSQLUsageMetrics-16    	     180	   6503225 ns/op
BenchmarkSQLUsageMetrics-16    	     172	   6594906 ns/op
BenchmarkSQLUsageMetrics-16    	     154	   6556032 ns/op

Two separate queries:

BenchmarkSQLUsageMetrics-16    	     132	   8800545 ns/op
BenchmarkSQLUsageMetrics-16    	     122	   8808408 ns/op
BenchmarkSQLUsageMetrics-16    	     118	   9132146 ns/op
BenchmarkSQLUsageMetrics-16    	     120	   8762194 ns/op

I'll update to use the inner join 👍

select um.TABLESPACE_NAME, um.USED_SPACE, um.TABLESPACE_SIZE, ts.BLOCK_SIZE
FROM DBA_TABLESPACE_USAGE_METRICS um INNER JOIN DBA_TABLESPACES ts
ON um.TABLESPACE_NAME = ts.TABLESPACE_NAME`
)

type dbProviderFunc func() (*sql.DB, error)
Expand All @@ -48,7 +50,6 @@ type clientProviderFunc func(*sql.DB, string, *zap.Logger) dbClient

type scraper struct {
statsClient dbClient
tablespaceMaxSpaceClient dbClient
tablespaceUsageClient dbClient
systemResourceLimitsClient dbClient
sessionCountClient dbClient
Expand Down Expand Up @@ -88,7 +89,6 @@ func (s *scraper) start(context.Context, component.Host) error {
s.sessionCountClient = s.clientProviderFunc(s.db, sessionCountSQL, s.logger)
s.systemResourceLimitsClient = s.clientProviderFunc(s.db, systemResourceLimitsSQL, s.logger)
s.tablespaceUsageClient = s.clientProviderFunc(s.db, tablespaceUsageSQL, s.logger)
s.tablespaceMaxSpaceClient = s.clientProviderFunc(s.db, tablespaceMaxSpaceSQL, s.logger)
return nil
}

Expand Down Expand Up @@ -274,41 +274,49 @@ func (s *scraper) scrape(ctx context.Context) (pmetric.Metrics, error) {
}
}
}
if s.metricsBuilderConfig.Metrics.OracledbTablespaceSizeUsage.Enabled {

if s.metricsBuilderConfig.Metrics.OracledbTablespaceSizeUsage.Enabled ||
s.metricsBuilderConfig.Metrics.OracledbTablespaceSizeLimit.Enabled {
rows, err := s.tablespaceUsageClient.metricRows(ctx)
if err != nil {
scrapeErrors = append(scrapeErrors, fmt.Errorf("error executing %s: %w", tablespaceUsageSQL, err))
} else {
now := pcommon.NewTimestampFromTime(time.Now())
for _, row := range rows {
tablespaceName := row["TABLESPACE_NAME"]
err := s.mb.RecordOracledbTablespaceSizeUsageDataPoint(now, row["BYTES"], tablespaceName)
usedSpaceBlockCount, err := strconv.ParseInt(row["USED_SPACE"], 10, 64)
if err != nil {
scrapeErrors = append(scrapeErrors, err)
scrapeErrors = append(scrapeErrors, fmt.Errorf("failed to parse int64 for OracledbTablespaceSizeUsage, value was %s: %w", row["USED_SPACE"], err))
continue
}
}
}
}
if s.metricsBuilderConfig.Metrics.OracledbTablespaceSizeLimit.Enabled {
rows, err := s.tablespaceMaxSpaceClient.metricRows(ctx)
if err != nil {
scrapeErrors = append(scrapeErrors, fmt.Errorf("error executing %s: %w", tablespaceMaxSpaceSQL, err))
} else {
now := pcommon.NewTimestampFromTime(time.Now())
for _, row := range rows {
tablespaceName := row["TABLESPACE_NAME"]
var val int64
inputVal := row["VALUE"]
if inputVal == "" {
val = -1

tablespaceSizeOriginal := row["TABLESPACE_SIZE"]
var tablespaceSizeBlockCount int64
// Tablespace size should never be empty using the DBA_TABLESPACE_USAGE_METRICS query. This logic is done
// to preserve backward compatibility for with the original metric gathered from querying DBA_TABLESPACES
if tablespaceSizeOriginal == "" {
tablespaceSizeBlockCount = -1
} else {
val, err = strconv.ParseInt(inputVal, 10, 64)
tablespaceSizeBlockCount, err = strconv.ParseInt(tablespaceSizeOriginal, 10, 64)
if err != nil {
scrapeErrors = append(scrapeErrors, fmt.Errorf("failed to parse int64 for OracledbTablespaceSizeLimit, value was %s: %w", inputVal, err))
scrapeErrors = append(scrapeErrors, fmt.Errorf("failed to parse int64 for OracledbTablespaceSizeLimit, value was %s: %w", tablespaceSizeOriginal, err))
continue
}
}
s.mb.RecordOracledbTablespaceSizeLimitDataPoint(now, val, tablespaceName)

blockSize, err := strconv.ParseInt(row["BLOCK_SIZE"], 10, 64)
if err != nil {
scrapeErrors = append(scrapeErrors, fmt.Errorf("failed to parse int64 for OracledbBlockSize, value was %s: %w", row["BLOCK_SIZE"], err))
continue
}

s.mb.RecordOracledbTablespaceSizeUsageDataPoint(now, usedSpaceBlockCount*blockSize, tablespaceName)

if tablespaceSizeBlockCount < 0 {
s.mb.RecordOracledbTablespaceSizeLimitDataPoint(now, -1, tablespaceName)
} else {
s.mb.RecordOracledbTablespaceSizeLimitDataPoint(now, tablespaceSizeBlockCount*blockSize, tablespaceName)
}
}
}
}
Expand Down
31 changes: 23 additions & 8 deletions receiver/oracledbreceiver/scraper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,7 @@ var queryResponses = map[string][]metricRow{
sessionCountSQL: {{"VALUE": "1"}},
systemResourceLimitsSQL: {{"RESOURCE_NAME": "processes", "CURRENT_UTILIZATION": "3", "MAX_UTILIZATION": "10", "INITIAL_ALLOCATION": "100", "LIMIT_VALUE": "100"},
{"RESOURCE_NAME": "locks", "CURRENT_UTILIZATION": "3", "MAX_UTILIZATION": "10", "INITIAL_ALLOCATION": "-1", "LIMIT_VALUE": "-1"}},
tablespaceUsageSQL: {{"TABLESPACE_NAME": "SYS", "BYTES": "1024"}},
tablespaceMaxSpaceSQL: {{"TABLESPACE_NAME": "SYS", "VALUE": "1024"}},
tablespaceUsageSQL: {{"TABLESPACE_NAME": "SYS", "USED_SPACE": "111288", "TABLESPACE_SIZE": "3518587", "BLOCK_SIZE": "8192"}},
}

func TestScraper_Scrape(t *testing.T) {
Expand Down Expand Up @@ -76,11 +75,11 @@ func TestScraper_Scrape(t *testing.T) {
{
name: "no limit on tablespace",
dbclientFn: func(_ *sql.DB, s string, _ *zap.Logger) dbClient {
if s == tablespaceMaxSpaceSQL {
if s == tablespaceUsageSQL {
return &fakeDbClient{Responses: [][]metricRow{
{
{"TABLESPACE_NAME": "SYS", "VALUE": "1024"},
{"TABLESPACE_NAME": "FOO", "VALUE": ""},
{"TABLESPACE_NAME": "SYS", "TABLESPACE_SIZE": "1024", "USED_SPACE": "111288", "BLOCK_SIZE": "8192"},
{"TABLESPACE_NAME": "FOO", "TABLESPACE_SIZE": "", "USED_SPACE": "111288", "BLOCK_SIZE": "8192"},
},
}}
}
Expand All @@ -92,11 +91,11 @@ func TestScraper_Scrape(t *testing.T) {
{
name: "bad value on tablespace",
dbclientFn: func(_ *sql.DB, s string, _ *zap.Logger) dbClient {
if s == tablespaceMaxSpaceSQL {
if s == tablespaceUsageSQL {
return &fakeDbClient{Responses: [][]metricRow{
{
{"TABLESPACE_NAME": "SYS", "VALUE": "1024"},
{"TABLESPACE_NAME": "FOO", "VALUE": "ert"},
{"TABLESPACE_NAME": "SYS", "TABLESPACE_SIZE": "1024", "USED_SPACE": "111288", "BLOCK_SIZE": "8192"},
{"TABLESPACE_NAME": "FOO", "TABLESPACE_SIZE": "ert", "USED_SPACE": "111288", "BLOCK_SIZE": "8192"},
},
}}
}
Expand All @@ -106,6 +105,22 @@ func TestScraper_Scrape(t *testing.T) {
},
errWanted: `failed to parse int64 for OracledbTablespaceSizeLimit, value was ert: strconv.ParseInt: parsing "ert": invalid syntax`,
},
{
name: "Empty block size",
dbclientFn: func(_ *sql.DB, s string, _ *zap.Logger) dbClient {
if s == tablespaceUsageSQL {
return &fakeDbClient{Responses: [][]metricRow{
{
{"TABLESPACE_NAME": "SYS", "TABLESPACE_SIZE": "1024", "USED_SPACE": "111288", "BLOCK_SIZE": ""},
},
}}
}
return &fakeDbClient{Responses: [][]metricRow{
queryResponses[s],
}}
},
errWanted: `failed to parse int64 for OracledbBlockSize, value was : strconv.ParseInt: parsing "": invalid syntax`,
},
}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
Expand Down
Loading