[oracle] Missing total.bytes field in tablespace datastream #39787

niraj-elastic · 2024-06-03T10:34:39Z

What does this PR do

The field oracle.tablespace.space.total.bytes is absent from the Table Space Metric set.Description
The query used to retrieve the Table Space metric for other than TEMP space is missing the calculation for Total bytes based on the collected bytes of read and write operations. This calculation is present for TEMP spaces but not for other spaces.
Query: Link to the query

In this PR, logic of total.bytes is updated to match with oracle integration.

Due to above change following visualization will need to update. following is before and after change images.

Before:

After:

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have added tests that prove my fix is effective or that my feature works
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Related issues

Closes [Metricbeat][Oracle] Field oracle.tablespace.space.total.bytes is missing #38051

mergify · 2024-06-03T10:35:15Z

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @niraj-elastic? 🙏.
For such, you'll need to label your PR with:

The upcoming major version of the Elastic Stack
The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

…ts into oracle_total_bytes_fix

shmsr · 2024-06-04T18:51:05Z

x-pack/metricbeat/module/oracle/tablespace/data.go

+		dataFileCopy := dataFile
+		m.addDataFileData(&dataFileCopy, out)


Suggested change

dataFileCopy := dataFile

m.addDataFileData(&dataFileCopy, out)

dataFileCopy := dataFile

m.addDataFileData(&dataFileCopy, out)

This gotcha is no longer valid starting from Go 1.22 and above. See the first point of this link related to "for". Currently, for beats, we are using Go 1.21 but I am pretty sure we will move to 1.22 very soon.

So, for now, we can fix it. But after Go 1.22 this is no more required. Also, if you look deeper into how that dataFile is used; it is just being read and not written to and that's why we never faced an issue where passing the address of the var caused us an issue here.

A cleaner way to address it for now would be to do this (without creating a new var):

for i := range in.dataFiles { m.addDataFileData(&in.dataFiles[i], out) }

+1 to what is being suggested here and totally agree with using the cleaner way as it's a way to stay consistent with pointer semantics. However, IMO, as pointed out that datafile is not modified, and the fact that we may move to Go 1.22, we could simply keep the function unchanged(if we wish to), and it will still be compatible with the new version.
@shmsr what do you reckon?

@niraj-elastic made this change to make the linter happy. So, either we have to put a comment of nolint or make the change to keep the linter happy.

I am more inclined towards making the following change as suppose next month we are still in Go 1.21 and someone changes the code such that dataFiles is even being modified but we won't be able to get any lint errors as we have put a nolint comment.

for i := range in.dataFiles { m.addDataFileData(&in.dataFiles[i], out) }

I have updated the code with suggested changes.

x-pack/metricbeat/module/oracle/tablespace/data.go

devamanv · 2024-06-04T21:05:28Z

x-pack/metricbeat/module/oracle/tablespace/data.go

+		dataFileCopy := dataFile
+		m.addDataFileData(&dataFileCopy, out)


+1 to what is being suggested here and totally agree with using the cleaner way as it's a way to stay consistent with pointer semantics. However, IMO, as pointed out that datafile is not modified, and the fact that we may move to Go 1.22, we could simply keep the function unchanged(if we wish to), and it will still be compatible with the new version.
@shmsr what do you reckon?

x-pack/metricbeat/module/oracle/tablespace/data.go

devamanv · 2024-06-05T12:49:47Z

CHANGELOG.next.asciidoc

@@ -176,6 +176,7 @@ https://github.com/elastic/beats/compare/v8.8.1\...main[Check the HEAD diff]
 - Fix issue where beats may report incorrect metrics for its own process when running inside a container {pull}39627[39627]
 - Fix for MySQL/Performance - Query failure for MySQL versions below v8.0.1, for performance metric `quantile_95`. {pull}38710[38710]
 - Normalize AWS RDS CPU Utilization values before making the metadata API call. {pull}39664[39664]
+- Fixed query logic for temp and non-temp tablespaces in Oracle module. {issue}38051[38051] {pull}39787[39787]


Suggested change

- Fixed query logic for temp and non-temp tablespaces in Oracle module. {issue}38051[38051] {pull}39787[39787]

- Fix query logic for temp and non-temp tablespaces in Oracle module. {issue}38051[38051] {pull}39787[39787]

devamanv

Left a minor suggestion. Rest looks good to me.

agithomas · 2024-06-06T09:31:26Z

x-pack/metricbeat/module/oracle/tablespace/temp_free_space.go

 }

 func (e *tablespaceExtractor) tempFreeSpaceData(ctx context.Context) ([]tempFreeSpace, error) {
-	rows, err := e.db.QueryContext(ctx, "SELECT TABLESPACE_NAME, TABLESPACE_SIZE, ALLOCATED_SPACE, FREE_SPACE FROM DBA_TEMP_FREE_SPACE")
+	rows, err := e.db.QueryContext(ctx, `WITH sums AS ( SELECT (SELECT SUM(BYTES) FROM DBA_DATA_FILES) + (SELECT SUM(BYTES) FROM DBA_TEMP_FILES) AS TOTAL_SUM FROM dual ) SELECT t.TABLESPACE_NAME, s.TOTAL_SUM, t.ALLOCATED_SPACE, t.FREE_SPACE FROM DBA_TEMP_FREE_SPACE t, sums s `)


Can you consider something simple as below

SELECT t.TABLESPACE_NAME, (SELECT SUM(BYTES) FROM DBA_DATA_FILES) + (SELECT SUM(BYTES) FROM DBA_TEMP_FILES) AS TOTAL_SUM, t.ALLOCATED_SPACE, t.FREE_SPACE FROM DBA_TEMP_FREE_SPACE t;

Talking about Niraj's query:

See this. (You are using cross join with CTE and DBA_TEMP_FREE_SPACE).

Now that I read more and more on this I keep on finding something new.

In the query below, the scalar subquery is used to get the TOTAL_SUM from sums CTE compared to your query where CTE is cross-joined with DBA_TEMP_FREE_SPACE. From what I read in most places, scalar subquery seems to be generally a better choice in terms of perf and readability. For example, As the same scalar subquery is running for each row, Oracle might as well cache it and keep using it. In the case of cross joins, it takes up a lot of memory to create a temp table to do the join (unless the DB optimizes it).

Find more here: See: https://stackoverflow.com/a/55192080/5821408 and https://asktom.oracle.com/Misc/oramag/on-caching-and-evangelizing-sql.html

Try this query?

WITH sums AS ( SELECT (SELECT SUM(BYTES) FROM DBA_DATA_FILES) + (SELECT SUM(BYTES) FROM DBA_TEMP_FILES) AS TOTAL_SUM FROM dual ) SELECT t.TABLESPACE_NAME, (SELECT TOTAL_SUM FROM sums) AS TOTAL_SUM, t.ALLOCATED_SPACE, t.FREE_SPACE FROM DBA_TEMP_FREE_SPACE t;

This query will calculate the total sum separately and display it as a single value for each row. No cross joins.

agithomas

Query improvement suggested

x-pack/metricbeat/module/oracle/tablespace/used_and_free_space.go

agithomas

More query improvements suggested

kush-elastic

I don't have expertise in Oracle, so I only reviewed the codebase changes.
LGTM!

agithomas · 2024-06-10T02:28:36Z

The aggregation function used for the visualization is Average.
Can Max() be a better option here?

agithomas

LGTM!

fix total.bytes for tablespace

5ec5122

niraj-elastic requested a review from a team as a code owner June 3, 2024 10:34

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Jun 3, 2024

mergify bot assigned niraj-elastic Jun 3, 2024

update changelog

4395915

kush-elastic added Metricbeat Metricbeat Module:beat The beat Metricbeat module Team:Obs-InfraObs Label for the Observability Infrastructure Monitoring team labels Jun 3, 2024

botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Jun 3, 2024

kush-elastic requested a review from agithomas June 3, 2024 10:51

kush-elastic and others added 5 commits June 3, 2024 16:21

Merge branch 'main' into oracle_total_bytes_fix

6b89212

fix CI errors

2e7a998

Merge branch 'oracle_total_bytes_fix' of github.com:niraj-elastic/bea…

683b39d

…ts into oracle_total_bytes_fix

fix CI

2ce18ac

fix CI issues

0873951

shmsr reviewed Jun 4, 2024

View reviewed changes

devamanv reviewed Jun 4, 2024

View reviewed changes

x-pack/metricbeat/module/oracle/tablespace/data.go Show resolved Hide resolved

devamanv reviewed Jun 5, 2024

View reviewed changes

devamanv added the bugfix label Jun 5, 2024

address review comments

296ed24

niraj-elastic requested review from shmsr and devamanv June 5, 2024 10:24

address review comment

f99a0a0

devamanv reviewed Jun 5, 2024

View reviewed changes

devamanv approved these changes Jun 5, 2024

View reviewed changes

update changelog

c90771f

agithomas reviewed Jun 6, 2024

View reviewed changes

x-pack/metricbeat/module/oracle/tablespace/used_and_free_space.go Outdated Show resolved Hide resolved

agithomas reviewed Jun 6, 2024

View reviewed changes

kush-elastic approved these changes Jun 6, 2024

View reviewed changes

address review comment

f0d0229

update visualization

4b5160d

shmsr approved these changes Jun 12, 2024

View reviewed changes

agithomas approved these changes Jun 13, 2024

View reviewed changes

update visualization name

d092845

ali786XI merged commit de63284 into elastic:main Jun 13, 2024
19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[oracle] Missing total.bytes field in tablespace datastream #39787

[oracle] Missing total.bytes field in tablespace datastream #39787

niraj-elastic commented Jun 3, 2024 •

edited by devamanv

Loading

mergify bot commented Jun 3, 2024

shmsr Jun 4, 2024

devamanv Jun 4, 2024

shmsr Jun 5, 2024

niraj-elastic Jun 5, 2024

devamanv Jun 4, 2024

devamanv Jun 5, 2024 •

edited

Loading

devamanv left a comment

agithomas Jun 6, 2024

shmsr Jun 6, 2024 •

edited

Loading

agithomas left a comment

agithomas left a comment

kush-elastic left a comment

agithomas commented Jun 10, 2024

agithomas left a comment

		dataFileCopy := dataFile
		m.addDataFileData(&dataFileCopy, out)

	- Fixed query logic for temp and non-temp tablespaces in Oracle module. {issue}38051[38051] {pull}39787[39787]
	- Fix query logic for temp and non-temp tablespaces in Oracle module. {issue}38051[38051] {pull}39787[39787]

[oracle] Missing total.bytes field in tablespace datastream #39787

[oracle] Missing total.bytes field in tablespace datastream #39787

Conversation

niraj-elastic commented Jun 3, 2024 • edited by devamanv Loading

What does this PR do

Checklist

Related issues

mergify bot commented Jun 3, 2024

shmsr Jun 4, 2024

Choose a reason for hiding this comment

devamanv Jun 4, 2024

Choose a reason for hiding this comment

shmsr Jun 5, 2024

Choose a reason for hiding this comment

niraj-elastic Jun 5, 2024

Choose a reason for hiding this comment

devamanv Jun 4, 2024

Choose a reason for hiding this comment

devamanv Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

devamanv left a comment

Choose a reason for hiding this comment

agithomas Jun 6, 2024

Choose a reason for hiding this comment

shmsr Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

agithomas left a comment

Choose a reason for hiding this comment

agithomas left a comment

Choose a reason for hiding this comment

kush-elastic left a comment

Choose a reason for hiding this comment

agithomas commented Jun 10, 2024

agithomas left a comment

Choose a reason for hiding this comment

niraj-elastic commented Jun 3, 2024 •

edited by devamanv

Loading

devamanv Jun 5, 2024 •

edited

Loading

shmsr Jun 6, 2024 •

edited

Loading