-
Notifications
You must be signed in to change notification settings - Fork 2.8k
[receiver/sqlserver] Remove warning on resource attributes #38831
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
[receiver/sqlserver] Remove warning on resource attributes #38831
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but are we enabling those attributes then?
2401522
to
5a898b9
Compare
I've clarified in the changelog notes. This does not change any functional behavior of the receiver. The resource attributes were disabled before this change, and will continue to be. This is only removing the warning 👍 I figured including a changelog may be helpful as users may wonder why they're not getting a warning message anymore, or if the resource attributes are actually enabled by default as the warning told them they would be. |
Co-authored-by: Curtis Robert <[email protected]>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description We removed 2 warnings in #38831 , and this PR updates corresponding test on Windows. cc @atoulme @crobert-1 <!-- Issue number (e.g. #1234) or full URL to issue, if applicable. --> #### Link to tracking issue Fixes #38842 <!--Describe what testing was performed and which tests were added.--> #### Testing Updated <!--Describe the documentation added.--> #### Documentation n/a <!--Please delete paragraphs that you did not use before submitting.-->
…metry#38831) <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description The `server.address` and `server.port` resource attributes in the SQL Server receiver had warnings that they will be enabled by default in a future release. These resource attributes are being set by simply passing through config option values. The config options, `server` and `port`, are optional. Also, the SQL Server receiver supports scraping Windows Performance counters for metrics, which render these resource attributes meaningless. For these reasons, the resource attributes should continue to be disabled by default. There may be some way to get these resource attributes from windows perf counters, but that is a separate work item that would be required before enabling. If that work gets done at some point we can revisit enabling these by default. Related: open-telemetry#35183 --------- Co-authored-by: Antoine Toulme <[email protected]>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description We removed 2 warnings in open-telemetry#38831 , and this PR updates corresponding test on Windows. cc @atoulme @crobert-1 <!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. --> #### Link to tracking issue Fixes open-telemetry#38842 <!--Describe what testing was performed and which tests were added.--> #### Testing Updated <!--Describe the documentation added.--> #### Documentation n/a <!--Please delete paragraphs that you did not use before submitting.-->
Description
The
server.address
andserver.port
resource attributes in the SQL Server receiver had warnings that they will be enabled by default in a future release. These resource attributes are being set by simply passing through config option values. The config options,server
andport
, are optional. Also, the SQL Server receiver supports scraping Windows Performance counters for metrics, which render these resource attributes meaningless. For these reasons, the resource attributes should continue to be disabled by default.There may be some way to get these resource attributes from windows perf counters, but that is a separate work item that would be required before enabling. If that work gets done at some point we can revisit enabling these by default.
Related: #35183