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

more modular consumables description #63

Merged
merged 2 commits into from
Oct 9, 2020

Conversation

guillomovitch
Copy link
Contributor

See agent PR #660 for original discussion

@g-bougard
Copy link
Contributor

Hi,
on the agent side we are looking for the "MAX" value and we just put a value as a percentage. In that case we may not set "UNIT". So I think the spec should clarify that point or a <MAX>100</MAX> for example may be added. So I'm not sure what's the best: leaving <UNIT/> empty or add a <MAX/> node.
The point is also would the plugin (or users) prefer to know the usage in percent or the unit and the max possible absolute value ?

@g-bougard g-bougard self-requested a review March 26, 2019 09:53
@guillomovitch
Copy link
Contributor Author

on the agent side we are looking for the "MAX" value and we just put a value as a percentage

Only if maximum value is available, otherwise current value is used, with a unit. Keeping the same behaviour with the new specification would produce the following results:

  1. <value>95</value><unit>%</unit>
  2. <value>95</value><unit>cm</unit>

As an alternative, this computation could be made server-side by transmitting raw values, ie:

  1. <current>95</current><max>100</max>
  2. <current>95</current><unit>cm</unit>

@g-bougard
Copy link
Contributor

g-bougard commented Apr 5, 2019

My thought about <MAX/> was more to handle cases where MAX <> 100 and UNIT <> '%'. I agree MAX could be optional if unit is %.
So I would update the spec like this:

...
              <!ELEMENT UNIT (#PCDATA)>
              <!-- use MAX if available and UNIT <> % -->
              <!ELEMENT MAX (#PCDATA)>

@guillomovitch
Copy link
Contributor Author

guillomovitch commented Apr 5, 2019

use MAX if available and UNIT <> %

I'm not sure to follow you here, as this double condition doesn't match current agent behaviour.

If a value is available for max OID (.1.3.6.1.2.1.43.11.1.1.8.1), the agent currently assumes the current consumable level is a fraction ($current * 100 / $max), and ignores the unit OID (.1.3.6.1.2.1.43.11.1.1.7.1) value, which doesn't include '%' anyway AFAIK.

Consider two different consumables, with the following SNMP values:

consumableA:

  • current: 50
  • max: 100
  • unit: whatever

consumableB:

  • current: 50
  • max: -2
  • unit: 16

With the current agent behaviour, the consumable A should be reported either by forging a value for unit, to make interpretation easier:

<consumable>
  <current_value>50</current_value>
  <unit>%</unit>
</consumable>

Let's call this scenario A.

Or not reporting unit at all, and let the information consumer deduce it's a computed fraction:

<consumable>
  <current_value>50</current_value>
</consumable>

Let's call this scenario B.

However, if we change the agent behaviour to report raw values directly, it seems safer to not report the unit at all, ie:

<consumable>
  <current_value>50</current_value>
  <max_value>100</max_value>
</consumable>

Let's call this scenario C.

In all case, consumable B would be reported as:

<consumable>
  <current_value>50</current_value>
  <unit>cm</unit>
</consumable>

@g-bougard
Copy link
Contributor

I agree with you. Maybe a more appropriate comment would be:

<!-- use MAX if available, or assume it's 100 when UNIT is not set -->

@g-bougard g-bougard force-pushed the better-consumable-specification branch from bbc1891 to 84a3e10 Compare October 9, 2020 12:48
@g-bougard g-bougard merged commit e276af4 into master Oct 9, 2020
@g-bougard g-bougard deleted the better-consumable-specification branch October 9, 2020 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants