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

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

Merged
merged 13 commits into from
Jun 13, 2024

Conversation

niraj-elastic
Copy link
Contributor

@niraj-elastic niraj-elastic commented Jun 3, 2024

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:
before oracle

After:
After oracle

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

@niraj-elastic niraj-elastic requested a review from a team as a code owner June 3, 2024 10:34
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Jun 3, 2024
Copy link
Contributor

mergify bot commented Jun 3, 2024

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

@kush-elastic 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 botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Jun 3, 2024
@kush-elastic kush-elastic requested a review from agithomas June 3, 2024 10:51
Comment on lines 44 to 45
dataFileCopy := dataFile
m.addDataFileData(&dataFileCopy, out)
Copy link
Member

Choose a reason for hiding this comment

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

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)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

+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?

Copy link
Member

Choose a reason for hiding this comment

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

@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)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the code with suggested changes.

Comment on lines 44 to 45
dataFileCopy := dataFile
m.addDataFileData(&dataFileCopy, out)
Copy link
Contributor

Choose a reason for hiding this comment

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

+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?

@devamanv devamanv added the bugfix label Jun 5, 2024
@niraj-elastic niraj-elastic requested review from shmsr and devamanv June 5, 2024 10:24
@@ -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]
Copy link
Contributor

@devamanv devamanv Jun 5, 2024

Choose a reason for hiding this comment

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

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]

Copy link
Contributor

@devamanv devamanv left a comment

Choose a reason for hiding this comment

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

Left a minor suggestion. Rest looks good to me.

}

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 `)
Copy link
Contributor

Choose a reason for hiding this comment

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

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;

Copy link
Member

@shmsr shmsr Jun 6, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@agithomas agithomas left a comment

Choose a reason for hiding this comment

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

Query improvement suggested

Copy link
Contributor

@agithomas agithomas left a comment

Choose a reason for hiding this comment

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

More query improvements suggested

Copy link
Collaborator

@kush-elastic kush-elastic left a comment

Choose a reason for hiding this comment

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

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

@agithomas
Copy link
Contributor

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

Copy link
Contributor

@agithomas agithomas left a comment

Choose a reason for hiding this comment

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

LGTM!

@ali786XI ali786XI merged commit de63284 into elastic:main Jun 13, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Metricbeat Metricbeat Module:beat The beat Metricbeat module Team:Obs-InfraObs Label for the Observability Infrastructure Monitoring team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Metricbeat][Oracle] Field oracle.tablespace.space.total.bytes is missing
6 participants