-
Notifications
You must be signed in to change notification settings - Fork 1.5k
add infra_mode tag to vsphere metric #21730
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
base: master
Are you sure you want to change the base?
Conversation
This PR is the equivalent of DataDog/datadog-agent#42081 for vsphere metrics.
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.
Hi @arbll I left a couple of questions. Just make sure to update the tests to ensure they are testing the current behavior.
I removed the no-changelog tag as this would not qualify for that use case. Any change that is going to be included in a future release of the agent needs to include a changelog that adds visibility to what has been modified both for us and for our customers.
Thanks!
My Feedback Legend
Here's a quick guide to the prefixes I use in my comments:
praise: no action needed, just celebrate!
note: just a comment/information, no need to take any action.
question: I need clarification or I'm seeking to understand your approach.
nit: A minor, non-blocking issue (e.g., style, typo). Feel free to ignore.
suggestion: I'm proposing an improvement. This is optional but recommended.
request: A change I believe is necessary before this can be merged.
The only blocking comments are request, any other type of comment can be applied at discretion of the developer.
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.
Added a comment about something I'm not sure about.
Also, curious if we could add a test for this
| tags.append('infra_mode:{}'.format(infra_mode)) | ||
|
|
||
| value = valid_values[-1] | ||
| if metric_name in PERCENT_METRICS: |
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.
Unsure when the vsphere. prefix is added to the collected metric_name (the logic here matches the metric_name against PERCENT_METRICS, which does not have this prefix, see code )
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.
The vsphere. prefix is added by the __NAMESPACE__ attribute, see:
| __NAMESPACE__ = 'vsphere' |
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.
Fixed this but will doublecheck with the live test that it's working properly
Codecov Report❌ Patch coverage is Additional details and impacted files🚀 New features to boost your workflow:
|
This PR is the equivalent of DataDog/datadog-agent#42081 for vsphere metrics.