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

Clarify implications of negative observations for Sum #193

Open
beorn7 opened this issue Mar 25, 2021 · 6 comments
Open

Clarify implications of negative observations for Sum #193

beorn7 opened this issue Mar 25, 2021 · 6 comments

Comments

@beorn7
Copy link
Member

beorn7 commented Mar 25, 2021

The RFC is very clear about the Sum of a Histogram being a counter: "Semantically, Sum, and buckets values are counters…".

It later states: "Negative threshold buckets MAY be used, but then the Histogram MetricPoint MUST NOT contain a sum value as it would no longer be a counter semantically."

However, the absence of negative buckets is not sufficient to guarantee that Sum has counter semantics. You could still put a negative measurement into a Histogram with only positive bucket boundaries. I believe the relation is stricter, more like: "A Histogram MAY receive negative measurements, but any such Histogram MUST NOT contain a Sum value as it would no longer be a counter semantically."

The Sum of a Summary is a counter, too: "Semantically, Count and Sum values are counters…" However, the issue of negative measurements is not even mentioned for Summaries. I suggest a similar wording as above, i.e. you MUST get rid of the Sum to receive negative measurements.

(I think I mentioned the above in my comments when reviewing the RFC in an earlier stage. Perhaps it got lost. Or perhaps I'm missing something. My apologies in advance if the latter is the case.)

@brian-brazil
Copy link
Contributor

I believe the relation is stricter, more like

Yes, that's what the spec says by saying that the Sum has counter semantics. The text there is more stating an implication of that.

However, the issue of negative measurements is not even mentioned for Summaries.

This was previously discussed, and it was decided not to allow negative observations for Summaries so the spec already covers that.

@beorn7
Copy link
Member Author

beorn7 commented Mar 25, 2021

Yes, that's what the spec says by saying that the Sum has counter semantics.

Hmm, OK. But I think that's quite implicit, and it's quite hard for all but the most knowledgeable readers to get from that statement to "you cannot have negative measurement".

The text there is more stating an implication of that.

With that stated implication, it gets even harder to realize. The wording "…but then…" could easily be understand as "…then and only then…". This wrong understanding is further reinforced by the statement about the GaugeHistogram: "If negative threshold buckets are present, then sum MAY be negative." This implies that without negative threshold buckets, Sum cannot be negative (which is a weird and needless restriction, IMHO).

In any case, I recommend to state explicitly what is implied. Even with my deep understanding of the matter, I'm not quite sure what is actually intended. Are negative observations without negative threshold buckets OK if I omit the Sum? Or are negative observations only allowed if there is at least one negative threshold bucket?

it was decided not to allow negative observations for Summaries so the spec already covers that.

OK, then that's what I missed about Summaries. But to be honest, I still fail to find any statement about negative observations being forbidden for Summaries. Could you point out the relevant section?

@brian-brazil
Copy link
Contributor

Are negative observations without negative threshold buckets OK if I omit the Sum?

As worded currently, I'd read it as being permitted though it's a bit of a corner case. The current text specifies only that having negative buckets with a sum is not permitted, as doesn't really make sense as you'd not be allowed use those negative buckets.

GaugeHistogram: "If negative threshold buckets are present, then sum MAY be negative." This implies that without negative threshold buckets, Sum cannot be negative (which is a weird and needless restriction, IMHO).

Hmm, you might have a point there. That feels like it might be an inconsistency, but we're very much in corner case land as you're expecting negative observations yet don't have negative buckets.

OK, then that's what I missed about Summaries. But to be honest, I still fail to find any statement about negative observations being forbidden for Summaries. Could you point out the relevant section?

As you already pointed to "Semantically, Count and Sum values are counters" which is elsewhere defined to be monotonically non-decreasing over time.

@beorn7
Copy link
Member Author

beorn7 commented Mar 26, 2021

As you already pointed to "Semantically, Count and Sum values are counters" which is elsewhere defined to be monotonically non-decreasing over time.

And as I also said before, getting from there to "observations MUST NOT be negative" will be quite hard for all but the most knowledgeable readers. I recommend to explicitly state this. It's just one simple sentence with a lot of value.

@RichiH
Copy link
Member

RichiH commented Mar 26, 2021

While the normative text should be rather terse, I like to err on the side of more considerations, not fewer. Do you have specific wording in mind?

@beorn7
Copy link
Member Author

beorn7 commented Mar 26, 2021

Yes, see the original issue: "A Histogram MAY receive negative measurements, but any such Histogram MUST NOT contain a Sum value as it would no longer be a counter semantically." (You could then even leave out the restriction for negative threshold buckets, in the interest of making the text terser. It would then be technically correct to have unused negative threshold buckets and a Sum. Such a setup would not make sense, but it wouldn't break anything either, while with the current text, misunderstandings are possible that will break things.)

Similarly, I'd write: "A Summary MUST NOT receive negative measurements as its Sum value would no longer be a counter semantically."

And finally, I'd simply remove the constraint on the negative Sum in the GaugeHistogram. That doesn't break anything and makes the text shorter (but it would actually change the spec, while the other suggestions are just expressing the original intent more explicitly).

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

No branches or pull requests

3 participants