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

Added more performance metrics with the existing query #37883

Merged
merged 1 commit into from
Mar 5, 2025
Merged

Added more performance metrics with the existing query #37883

merged 1 commit into from
Mar 5, 2025

Conversation

samiura
Copy link
Contributor

@samiura samiura commented Feb 12, 2025 •
edited
Loading

Description

Added more performance metrics with the existing query. These metrics were already fetched with the existing query but were not registered as metrics.

  • sqlserver.database.backup_or_restore.rate
  • sqlserver.replica.sent.rate
  • sqlserver.replica.received.rate
  • sqlserver.database.execution.errors
  • sqlserver.table.count
  • sqlserver.page.free_list_stalls.rate
  • sqlserver.database.tempdb.space
  • sqlserver.database.full_scan.rate
  • sqlserver.index.search.rate
  • sqlserver.login.rate
  • sqlserver.logout.rate
  • sqlserver.database.deadlock.rate
  • sqlserver.database.mirror_write_transaction.rate
  • sqlserver.memory.grants_pending.count
  • sqlserver.page.lookup.rate
  • sqlserver.transaction.delay
  • sqlserver.memory.usage
  • sqlserver.database.tempdb.version_store.size
  • sqlserver.lock.timeout.rate
Screenshot 2025-02-24 at 10 28 34

Link to tracking issue

Fixes

Testing

Tested against Micrsoft SQL Server Standard 2022

Documentation

Updated the documentation.

@github-actions github-actions bot requested a review from StefanKurek February 12, 2025 21:41
@samiura samiura force-pushed the added-more-sqlserver-receiver-preformace-metrics branch from 2462990 to 177bb5d Compare February 12, 2025 21:45
@samiura samiura mentioned this pull request Feb 12, 2025
Open
@samiura samiura force-pushed the added-more-sqlserver-receiver-preformace-metrics branch 3 times, most recently from 2f5a71a to 79464d2 Compare February 12, 2025 23:46
@samiura samiura marked this pull request as ready for review February 12, 2025 23:46
@samiura samiura requested a review from a team as a code owner February 12, 2025 23:46
@samiura samiura force-pushed the added-more-sqlserver-receiver-preformace-metrics branch 3 times, most recently from 581d037 to 614e96e Compare February 13, 2025 17:17
crobert-1
crobert-1 reviewed Feb 13, 2025
View reviewed changes
receiver/sqlserverreceiver/metadata.yaml
Copy link
Member

Choose a reason for hiding this comment

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

I think the naming and format of the newly introduced metrics should be changed pretty considerably to match the spec.

I think the first thing we want to do is to break metrics down into namespaces, so related metrics can be found more easily.

We also want to have one metric for multi-directional, or multi-state information. View sqlserver.database.count as an example, it's one metric with a datapoint for each possible state.

The spec has quite a few rules around metric naming and organization that are not being met here, so it would be helpful to take a look and address as many as possible.

Overall we just want to make sure these metrics are as easy to find and understand as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@crobert-1 Yes! I was not happy with the naming either and will work on it along with the valuable issues addressed below by Stefan.

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 pushed a new commit and tried to follow the guidelines regarding naming style but open to suggestion.

StefanKurek
StefanKurek reviewed Feb 13, 2025
View reviewed changes
Copy link
Contributor

@StefanKurek StefanKurek left a comment

Choose a reason for hiding this comment

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

I plan on looking through all of the names soon.

@samiura samiura force-pushed the added-more-sqlserver-receiver-preformace-metrics branch 13 times, most recently from 99510f0 to 46aa8c9 Compare February 17, 2025 19:54
@samiura samiura force-pushed the added-more-sqlserver-receiver-preformace-metrics branch 3 times, most recently from a65b5d9 to 5ebf603 Compare February 27, 2025 18:24
@samiura samiura requested a review from StefanKurek February 27, 2025 18:25
@samiura samiura force-pushed the added-more-sqlserver-receiver-preformace-metrics branch from 5ebf603 to 311311e Compare February 27, 2025 20:33
StefanKurek
StefanKurek reviewed Mar 3, 2025
View reviewed changes
Copy link
Contributor

@StefanKurek StefanKurek left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think just one more change (which is actually me going back on a previous comment as seen here #37883 (comment) and then I'm happy with it. Thanks for the work!

@samiura samiura force-pushed the added-more-sqlserver-receiver-preformace-metrics branch 5 times, most recently from eb6204f to 6162b63 Compare March 4, 2025 16:52
@samiura
Copy link
Contributor Author

samiura commented Mar 4, 2025 •
edited
Loading

I think just one more change (which is actually me going back on a previous comment as seen here #37883 (comment) and then I'm happy with it. Thanks for the work!

@StefanKurek I have already changed it to gauge from the comment you made. Do you want me to revert to sum non-monotonic? I am bit confused here.

@samiura samiura requested a review from StefanKurek March 4, 2025 17:00
@samiura samiura force-pushed the added-more-sqlserver-receiver-preformace-metrics branch 9 times, most recently from 38da9d1 to 696acc9 Compare March 5, 2025 13:59
StefanKurek
StefanKurek approved these changes Mar 5, 2025
View reviewed changes
Copy link
Contributor

@StefanKurek StefanKurek left a comment

Choose a reason for hiding this comment

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

I think this looks good! Thanks for all the work.

@samiura samiura force-pushed the added-more-sqlserver-receiver-preformace-metrics branch 2 times, most recently from eb8832f to f7b966e Compare March 5, 2025 16:37
@samiura
Added more performance metrics with the existing query
1415621 
Signed-off-by: Samiur Arif <samiur.arif@sumologic.com>

Update .chloggen/added-more-sqlserver-receiver-preformace-metrics.yaml

Co-authored-by: Curtis Robert <crobert@splunk.com>

Signed-off-by: Samiur Arif <samiur.arif@sumologic.com>

Signed-off-by: Samiur Arif <samiur.arif@sumologic.com>

Signed-off-by: Samiur Arif <samiur.arif@sumologic.com>

Signed-off-by: Samiur Arif <samiur.arif@sumologic.com>

Addressing previous PR comments

Signed-off-by: Samiur Arif <samiur.arif@sumologic.com>

Addressing previous PR comments

Signed-off-by: Samiur Arif <samiur.arif@sumologic.com>

fixed unit tests

Signed-off-by: Samiur Arif <samiur.arif@sumologic.com>

minor name changes

Signed-off-by: Samiur Arif <samiur.arif@sumologic.com>

addressing previous PR comments

Signed-off-by: Samiur Arif <samiur.arif@sumologic.com>

addressing previous PR comments

Signed-off-by: Samiur Arif <samiur.arif@sumologic.com>
@samiura samiura force-pushed the added-more-sqlserver-receiver-preformace-metrics branch from f7b966e to 1415621 Compare March 5, 2025 16:58
atoulme
atoulme approved these changes Mar 5, 2025
View reviewed changes
Copy link
Contributor

@atoulme atoulme left a comment

Choose a reason for hiding this comment

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

LGTM, codeowner approved.

@atoulme atoulme merged commit f1f3dd8 into open-telemetry:main Mar 5, 2025
156 checks passed
@github-actions github-actions bot added this to the next release milestone Mar 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atoulme atoulme

@StefanKurek StefanKurek

@MovieStoreGuy MovieStoreGuy

@crobert-1 crobert-1

@fguimond fguimond

Assignees

@dashpole dashpole

Labels
receiver/sqlserver
Projects
None yet
Milestone
next release
Development

Successfully merging this pull request may close these issues.
6 participants
@samiura @atoulme @fguimond @StefanKurek @crobert-1 @dashpole