Skip to content

[receiver/sqlserver] Add collection interval for top query #40002

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

Conversation

sincejune
Copy link
Contributor

Description

This PR added collection_interval for top query to make the collection not too frequent:

  • This value must be a string readable by Golang's time.ParseDuration.
  • This value can only guarantee that the top queries are collected at most once in this interval.
    • For instance, you have global collection_interval as 10s and top_query_collection.collection_interval as 60s.
      • In this case, the default receiver scraper will still try to run in every 10 seconds.
      • However, the top queries collection will only run after 60 seconds have passed since the last collection.
    • For instance, you have global collection_interval as 10s and top_query_collection.collection_interval as 5s.
      • In this case, top_query_collection.collection_internal will make no effects to the collection

Link to tracking issue

n/a

Testing

Updated

Documentation

Updated

@sincejune sincejune requested review from crobert-1 and a team as code owners May 12, 2025 17:47
@github-actions github-actions bot added receiver/sqlserver Run Windows Enable running windows test on a PR labels May 12, 2025
@github-actions github-actions bot requested a review from StefanKurek May 12, 2025 17:48
@sincejune
Copy link
Contributor Author

This PR should not be part of 0.126.0 release.

Comment on lines +52 to +57
- This value can only guarantee that the top queries are collected at most once in this interval.
- For instance, you have global `collection_interval` as `10s` and `top_query_collection.collection_interval` as `60s`.
- In this case, the default receiver scraper will still try to run in every 10 seconds.
- However, the top queries collection will only run after 60 seconds have passed since the last collection.
- For instance, you have global `collection_interval` as `10s` and `top_query_collection.collection_interval` as `5s`.
- In this case, `top_query_collection.collection_internal` will make no effects to the collection
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels like it should be moved out from the attribute definitions, and into its own section that can explain it in more detail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This document is specifically intended for configurations, so it seems appropriate to include it here?

Copy link
Contributor

Choose a reason for hiding this comment

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

As a user who is new to the component, this is a lot of context provided as dot points. To help give a deeper background and context, it should be moved to its own section and expanded upon in a paragraph so examples can also be given.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't call this a blocking action, but as a user it would be nice to provide that extra context in a more naturally written form.

@sincejune sincejune marked this pull request as draft May 13, 2025 14:35
@sincejune sincejune marked this pull request as ready for review May 15, 2025 12:44
Copy link
Member

@crobert-1 crobert-1 left a comment

Choose a reason for hiding this comment

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

LGTM once comments are addressed 👍

@crobert-1 crobert-1 added ready to merge Code review completed; ready to merge by maintainers and removed ready to merge Code review completed; ready to merge by maintainers labels May 20, 2025
@crobert-1
Copy link
Member

I'll wait on ready to merge to confirm @MovieStoreGuy is okay with proceeding at this time. I'll add it again soon if there isn't any more comment. Since it's documentation it's pretty easy to fix later if necessary.

Comment on lines +52 to +57
- This value can only guarantee that the top queries are collected at most once in this interval.
- For instance, you have global `collection_interval` as `10s` and `top_query_collection.collection_interval` as `60s`.
- In this case, the default receiver scraper will still try to run in every 10 seconds.
- However, the top queries collection will only run after 60 seconds have passed since the last collection.
- For instance, you have global `collection_interval` as `10s` and `top_query_collection.collection_interval` as `5s`.
- In this case, `top_query_collection.collection_internal` will make no effects to the collection
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't call this a blocking action, but as a user it would be nice to provide that extra context in a more naturally written form.

@MovieStoreGuy MovieStoreGuy merged commit 5fa847a into open-telemetry:main May 22, 2025
371 of 407 checks passed
@github-actions github-actions bot added this to the next release milestone May 22, 2025
dragonlord93 pushed a commit to dragonlord93/opentelemetry-collector-contrib that referenced this pull request May 23, 2025
…metry#40002)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
This PR added `collection_interval` for top query to make the collection
not too frequent:
- This value must be a string readable by Golang's
[time.ParseDuration](https://pkg.go.dev/time#ParseDuration).
- This value can only guarantee that the top queries are collected at
most once in this interval.
- For instance, you have global `collection_interval` as `10s` and
`top_query_collection.collection_interval` as `60s`.
- In this case, the default receiver scraper will still try to run in
every 10 seconds.
- However, the top queries collection will only run after 60 seconds
have passed since the last collection.
- For instance, you have global `collection_interval` as `10s` and
`top_query_collection.collection_interval` as `5s`.
- In this case, `top_query_collection.collection_internal` will make no
effects to the collection

<!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. -->
#### Link to tracking issue
n/a

<!--Describe what testing was performed and which tests were added.-->
#### Testing
Updated

<!--Describe the documentation added.-->
#### Documentation
Updated

<!--Please delete paragraphs that you did not use before submitting.-->

---------

Co-authored-by: Curtis Robert <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
receiver/sqlserver Run Windows Enable running windows test on a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants