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.

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

feat(inputs.redfish): adds more chassis tags #13727

Merged
merged 6 commits into from
Aug 11, 2023

Conversation

steven-freed
Copy link
Contributor

@steven-freed steven-freed commented Aug 5, 2023

adds more chassis tags to the redfish input plugin. These chassis tags are applied to the underlying chassis links containing metrics such as Power and Thermal similar to how the implementation for Chassis.Location was done.

Required for all PRs

resolves #

adds more chassis tags to the redfish input plugin. These chassis tags are also applied to the underlying chassis links such as Power and Thermal similar to how the implementation for Chassis.Location was done.
@steven-freed steven-freed changed the title feat(input.redfish): adds more chassis tags feat(inputs.redfish): adds more chassis tags Aug 6, 2023
Copy link
Contributor

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @steven-freed! While the code looks good I do have concerns regarding backward-compatibility on the query side. If you add tags and feed the data to e.g. InfluxDB you can get different results for your query (e.g. when grouping by all tags) let alone the increased cardinality.

Therefore, I suggest to add a setting to add the new tag set with a default of false. I think that is not too hard given your nice cutting of the code... What do you think?

@srebhan srebhan added feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Aug 7, 2023
@srebhan srebhan self-assigned this Aug 7, 2023
@steven-freed
Copy link
Contributor Author

steven-freed commented Aug 7, 2023

@srebhan thanks, happy to be here! I'm thinking of adding a setting called include_parent_tags to include the child link's (e.g. Thermal, Power) parent tags (e.g. Chassis). However, the way the previous version was written includes Chassis.Location tags along with the child link tags. Would it be ok if this compatibility is broken? (Only outputting the location tags if include_parent_tags=true) It would be cleaner and match the API spec more closely but if you'd rather I leave those extra location tags I can do that as well.

Also a good argument to increased cardinality would be that if a client really was concerned with cardinality then they should restrict tags fed to their input plug-in by using the "taginclude" telegraf input settings.

@srebhan
Copy link
Contributor

srebhan commented Aug 8, 2023

We cannot break current users @steven-freed. However, how about having something like

  include_tag_set = ["thermal", "power", "chassis.location", ...]

with a granularity that matches what is there and make your tag-set a new element in that array? This way you could set that array to something that matches the current state and we can easily add new tag-sets if necessary...

@steven-freed
Copy link
Contributor Author

I see. Ok that sounds good. We can default to the current tag sets as you've shown above.

adds new configuration setting include_tag_sets. this allows for limiting tags per metric but also allows for including redfish link parent data where that parent data may only have useful tags but no metrics.
@steven-freed
Copy link
Contributor Author

I've left out chassis.thermal and chassis.power from tag sets because this would allow for users to include a metric in their telegraf.conf that may not have any tags.
For example:
namepass = ["redfish_thermal_fans"] metric may not have any tags if the user sets include_tag_sets = ["chassis.power"]

To resolve this issue, I've modeled the code so that tag sets can be used to add more tags rather than restrict tags. To ensure backwards compatibility I've defaulted the tag sets to include chassis.location. Thanks for the recommendations!

Copy link
Contributor

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

@steven-freed some small comments from my side, but the overall code looks quite good already.

plugins/inputs/redfish/redfish.go Outdated Show resolved Hide resolved
plugins/inputs/redfish/sample.conf Outdated Show resolved Hide resolved
plugins/inputs/redfish/redfish.go Outdated Show resolved Hide resolved
plugins/inputs/redfish/redfish.go Outdated Show resolved Hide resolved
plugins/inputs/redfish/redfish.go Outdated Show resolved Hide resolved
…sample.conf

cleanup tagset logic by moving default tag set to init rather than Init. updated README.md and sample.conf with all available tag set options. unexport tagSet field in Redfish struct.
Copy link
Contributor

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

The config should show the actual default values but mention the available options. Did that for you and will push to get this over the finish line.

Thanks for your contribution @steven-freed!

plugins/inputs/redfish/README.md Outdated Show resolved Hide resolved
plugins/inputs/redfish/sample.conf Outdated Show resolved Hide resolved
@telegraf-tiger
Copy link
Contributor

@srebhan srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Aug 11, 2023
@srebhan srebhan assigned powersj and unassigned srebhan Aug 11, 2023
@powersj powersj merged commit 2f1edfb into influxdata:master Aug 11, 2023
22 checks passed
@powersj
Copy link
Contributor

powersj commented Aug 11, 2023

Thank you!

@github-actions github-actions bot added this to the v1.28.0 milestone Aug 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants