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

feat(inputs.sqlserver): adding data and log used space metrics for Azure SQL DB #12126

Merged
merged 5 commits into from
Nov 16, 2022

Conversation

dba-leshop
Copy link
Contributor

@dba-leshop dba-leshop commented Oct 30, 2022

Required for all PRs

  • Updated associated README.md.
  • Wrote appropriate unit tests => No additional unit tests required
  • Pull request title or commits are in conventional commit format

resolves #11406

Currently, available_storage_mb value doesn't include all available space (only space available in database file). Azure SQL DB available / used space takes into account allocated space as well. 3 new additional metrics related to data and log space usage for each Azure SQL DB including used_space_data_mb / total_log_mb / used_space_log_mb. They allow to compute database real space used and available space for both data and log.

@telegraf-tiger telegraf-tiger bot added area/sqlserver feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Oct 30, 2022
@dba-leshop
Copy link
Contributor Author

Queried updated according to comments

  • Adding columns used_space_data_mb and total_log_mb. For the latter max_size value - sys.database_files for getting transaction log file instead of sys.dm_db_log_space usage
  • no breaking changes for available_storage_mb column . Only formula has been updated

Copy link

@dimitri-furman dimitri-furman 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 couple of minor naming suggestions, looks good otherwise.

)
END AS [available_storage_mb]
END AS [available_storage_mb]
,(SELECT SUM(max_size) * 8 / (1024 * 1024) FROM sys.database_files WHERE type_desc = 'LOG') AS total_log_mb

Choose a reason for hiding this comment

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

Suggested change
,(SELECT SUM(max_size) * 8 / (1024 * 1024) FROM sys.database_files WHERE type_desc = 'LOG') AS total_log_mb
,(SELECT SUM(max_size) * 8 / (1024 * 1024) FROM sys.database_files WHERE type_desc = 'LOG') AS max_log_storage_mb

total_log_mb might be a bit ambiguous as a name. It's unclear if it refers to total log used size, total log allocated size, or maximum possible log size. Suggested name attempts to be more explicit. It is also consistent with the naming convention used with other columns (availaible_storage_mb, used_storage_mb).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. So to keep consistent, here my proposal:

max_storage_mb (to replace total_storage_mb) = maximum capacity for data
max_log_mb (to replace max_log_storage_mb) = maximum capacity for transaction log for the database tier
used_space_data_mb = space used within data files
available_storage_mb (keep this name to avoid breaking change) = Remaining free space that can be allocated regarding the maximum space provisioned for the database tier

Choose a reason for hiding this comment

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

total_storage_mb is an existing column, right? If we are keeping available_storage_mb for backward compatibility reasons, shouldn't we do that for total_storage_mb too?

The naming convention for all storage-related columns in this query could be improved for consistency and clarity, but in my view, we should prioritize compatibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, yes total_storage_mb already exists so we can't change it.
Last commit contains change for total_log_mb -> max_log_mb

@powersj powersj added the waiting for response waiting for response from contributor label Oct 31, 2022
@telegraf-tiger
Copy link
Contributor

Hello! I am closing this issue due to inactivity. I hope you were able to resolve your problem, if not please try posting this question in our Community Slack or Community Page. Thank you!

@telegraf-tiger telegraf-tiger bot closed this Nov 14, 2022
@powersj powersj reopened this Nov 14, 2022
@powersj
Copy link
Contributor

powersj commented Nov 14, 2022

@dimitri-furman can you give the current state a review?

@Trovalo if you have any thoughts on this I would be interested as well.

@telegraf-tiger telegraf-tiger bot removed the waiting for response waiting for response from contributor label Nov 14, 2022
@Trovalo
Copy link
Collaborator

Trovalo commented Nov 15, 2022

I've got nothing to say on the technical side, as Microsoft gave guidance on this one.
The only point left "hanging" is this one, about coherent naming
(current) used_space_data_mb vs used_storage_mb (suggested).

Honestly, I like more the current one, as it's more precise even if a little out of naming convention

@Hipska Hipska added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Nov 16, 2022
@Hipska Hipska removed the request for review from MyaLongmire November 16, 2022 08:41
Co-authored-by: Dimitri Furman <dfurman@microsoft.com>
@telegraf-tiger
Copy link
Contributor

@powersj powersj merged commit 58d7dfc into influxdata:master Nov 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/sqlserver feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Azure SQL DB - Improve available_storage_mb value
5 participants