-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Add full_server_name support as template variable to sqlserver template config key #20342
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
Add full_server_name support as template variable to sqlserver template config key #20342
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| # template: $env-$resolved_hostname:$port | ||
| # template: $full_server_name |
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.
This example exists to show how to use tags, why is it changed?
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.
Many users will simply wonder what's the best variable to use to name their sqlserver instance. For sqlserver the default should be $full_server_name in my opinion (if users are willing to diverge from the actual default of $resolved_hostname).
| # Keys of the static info cache, used to cache server info which does not change | ||
| STATIC_INFO_SERVERNAME = 'servername' | ||
| STATIC_INFO_INSTANCENAME = 'instancename' | ||
| STATIC_INFO_FULL_SERVERNAME = 'full_servername' |
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.
Nit: let's be consistent of if it's server_name or servername
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.
I thought I was being consistent here, since servername above is one word, yet its template variable is $server_name.
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.
So the static variable is full_servername and the actual template variable is full_server_name
Review from sethsamuel is dismissed. Related teams and files:
- database-monitoring-agent
- sqlserver/assets/configuration/spec.yaml
- sqlserver/datadog_checks/sqlserver/data/conf.yaml.example
This pull request introduces support for a new
$full_server_namevariable in the SQL Server integration for thetemplateconfig key, which represents the full server name as returned by@@SERVERNAME. The changes include updates to constants, configuration templates, caching mechanisms, and tests to incorporate this new field.Enhancements to SQL Server Integration:
Support for
full_server_name:STATIC_INFO_FULL_SERVERNAMEas a new constant insqlserver/datadog_checks/sqlserver/const.pyto represent the full server name.sqlserver/datadog_checks/sqlserver/data/conf.yaml.example) to includefull_server_nameas a new template option for identifying SQL Server instances.Updates to Static Information Cache:
load_static_informationinsqlserver/datadog_checks/sqlserver/sqlserver.pyto cache thefull_server_namevalue.database_identifierinsqlserver/datadog_checks/sqlserver/sqlserver.pyto useSTATIC_INFO_FULL_SERVERNAMEwhen applicable.''if its value isNoneto avoid seeingNonein the output.Test Coverage:
sqlserver/tests/test_unit.pyto validatefull_server_namefunctionality, including its usage in templates and caching. [1] [2]