-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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.apcupsd): Added new fields to apcupsd #12014
feat(inputs.apcupsd): Added new fields to apcupsd #12014
Conversation
…_time_on_battery_ns, last_transfer, number_transfers. Aligned order of fields in implementation, test and documentation
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.
Thanks for putting this up - one initial question about duplicate status
field?
plugins/inputs/apcupsd/README.md
Outdated
- model | ||
- fields: | ||
- status_flags ([status-bits][]) | ||
- status |
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.
Hmm having status
as a field and tag is not ideal. What is the difference in data?
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.
There is no difference in data. The disadvantage having the status as tag is that it may change, leading to different datasets for the same device. I thought about removing it from tags completely but decided to leave it there for backwards compatibility, avoiding breaking someone's dashboards.
EDIT: Probably this makes it more clear what I mean with "different datasets" when status changes:
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.
thanks for the example, let me chat with the team on Monday and get back to you
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.
after chatting with the team it seems historically we have not done this. Instead of someone needs a tag as a field or a field as a tag, then the correct thing is to use the converter processor.
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.
Thanks @powersj for your response. So do you suggest to remove the status
in the current implementation of this PR
- from the tags - having it as field only (where it should reside IMHO), breaking change
or - from the fields - keeping it as a tag, being backwards compatible?
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.
keeping it as a tag, being backwards compatible
This is the way
We try to keep everything as backwards compatible as possible.
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.
Well, fine for me. Removed status from fields again.
Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
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.
Thank you!
Required for all PRs
resolves #9761