<p dir="auto">I noticed that <code class="notranslate">ByteSizeValue</code> allows any input value, including negative values.<br>
When it comes to serializing it though, we use <code class="notranslate">writeVLong</code> which starts with an assertion <code class="notranslate">assert i &gt;= 0;</code>. I suspect this is the reason why in some cases we do not use <code class="notranslate">ByteSizeValue</code> instance but plain long values instead, in places like <code class="notranslate">OsStats</code> (because we default to <code class="notranslate">-1</code> which cannot be serialized) and we end up creating a new <code class="notranslate">ByteSizeValue</code> instance on demand for each get call.</p>
<p dir="auto">Should we change the way we serialize it or shall we introduce some validation at construction time in <code class="notranslate">ByteSizeValue</code>?</p>
<p dir="auto">See the discussion on <a class="issue-link js-issue-link" data-error-text="Failed to load title" data-id="168620521" data-permission-text="Title is private" data-url="https://github.com/elastic/elasticsearch/issues/19717" data-hovercard-type="pull_request" data-hovercard-url="/elastic/elasticsearch/pull/19717/hovercard" href="https://github.com/elastic/elasticsearch/pull/19717">#19717</a></p>
<p dir="auto">The stats api has some problems, which makes it hard to safely add new statistics or even keep the current ones working:</p>
<ul dir="auto">
<li>ByteSizeValue is currently used, but its not obvious how to handle "unsupported" or "unbounded" cases, while at the same time using this API. doing "interpretation" to 0 is confusing. Using ByteSizeValue itself, well, its totally unclear about what values it supports. Does it really support negatives? Does it really support -1? In one method you can see logic for e.g. <code class="notranslate">-1</code>, in other methods you can see they will be clearly broken if the value is negative. I assume there are missing unit tests as well as missing checks and missing docs and bugs.</li>
<li>Stats have missing unit tests. It would be flaky to test the results from the jvm, but simple unit tests can be written for e.g. serialization/deserialization logic. This is easy to do and plain old POJO testing, it can ensure all possible range of values (including exceptional cases) will work correctly.</li>
<li>The wrapper POJOs are missing docs. This makes it hard to work the code, since its not easy to correlate them to the MBean where they come from, and the methods on them to which mbean methods returned that piece of data. Adding <code class="notranslate">@see</code> can help, as well as trying to improve the consistency of naming...</li>
<li>Too much leniency: lots of <code class="notranslate">catch (Exception)</code> and allowing things to be null that are unnecessary. For example <code class="notranslate">ManagementFactory</code> methods document whether or not something is guaranteed to be there in the jvm, we shouldn't allow these to be null or catch Exceptions. If its not guaranteed, sure, but those are special cases. This leniency hides the fact, for example, that classloader statistics are currently just "dropped" across the wire.</li>
</ul>
<p dir="auto">I closed that PR because i don't think we should be piling on more statistics until we fix this stuff...</p>