Correctly handle empty labels. #594
Conversation
labels/labels.go
Outdated
| @@ -103,6 +103,23 @@ func (ls Labels) Map() map[string]string { | |||
| return m | |||
| } | |||
|
|
|||
| // WithoutEmpty returns the labelset without empty labels. | |||
| // May return the same the same labelset.. | |||
There was a problem hiding this comment.
"the same the same"?
brian-brazil
force-pushed
the
empty-labels
branch
from
7f76edc
to
b5604bd
Compare
There was a problem hiding this comment.
Potentially dumb questions (I'm not very familiar with prometheus/tsdb):
- Should the behavior be documented in the
Appenderinterface? (e.g. "labels with empty values are discarded during append") - Does the querier follow the same semantics? (if I query for labels with empty values, it means the same as not querying for that label)
labels/labels.go
Outdated
| func (ls Labels) WithoutEmpty() Labels { | ||
| for _, v := range ls { | ||
| if v.Value == "" { | ||
| els := make(Labels, 0, len(ls)) |
There was a problem hiding this comment.
els will be at least one shorter than ls. Thus, len(ls)-1 should be strictly better than len(ls).
There was a problem hiding this comment.
Agree, but potentially does not matter as there might be even more empty values in set.
There was a problem hiding this comment.
Even then we waste one less element if using len(ls)-1. That's what I mean by “strictly better”.
There was a problem hiding this comment.
Not that it matters a lot, of course. Performance-wise, that is.
In my way of viewing things, doing len(ls)-1 is actually a readability improvement. As the code reader, I start to think about possible reasons why len(ls) was used and why my understanding of the code (which suggests that at most len(ls)-1 will be needed) might be wrong.
If performance wouldn't matter at all, I would not pre-allocate anything for maximum readability.
In this case, I think len(ls)-1 > no pre-allocation > len(ls)
There was a problem hiding this comment.
Thanks for clarification, we are definitely on the same page @beorn7 (:
| @@ -103,6 +103,23 @@ func (ls Labels) Map() map[string]string { | |||
| return m | |||
| } | |||
|
|
|||
| // WithoutEmpty returns the labelset without empty labels. | |||
| // May return the same labelset. | |||
| func (ls Labels) WithoutEmpty() Labels { | |||
There was a problem hiding this comment.
Maybe my personal opinion, but I would say just function is better here as it has to be done only once at certain code logic.
Having this as Labels method indicates you need to do it a lot... but we don't want unnecceary run through all labels if not needed.
There was a problem hiding this comment.
If this turns out to be a bigger problem than I think, we'll need to sprinkle this across the codebase.
There was a problem hiding this comment.
I don't feel strongly either way.
labels/labels.go
Outdated
| func (ls Labels) WithoutEmpty() Labels { | ||
| for _, v := range ls { | ||
| if v.Value == "" { | ||
| els := make(Labels, 0, len(ls)) |
There was a problem hiding this comment.
We might be able to do it in less indent (more verbose), but not a blocker.
Yes, we go to a good bit of effort to do so. These are the general semantics of PromQL. |
Currently a time series with empty labels is not treated the same as one with missing labels. Currently this can only come from ALERTS&ALERT_FOR_STATE so it's unlikely anyone has actually hit it. Signed-off-by: Brian Brazil <[email protected]>
brian-brazil
force-pushed
the
empty-labels
branch
from
b5604bd
to
d49c159
Compare
|
Are there any other comments on this? |
Currently a time series with empty labels is not treated the same
as one with missing labels. Currently this can only come from
ALERTS&ALERT_FOR_STATE so it's unlikely anyone has actually hit it.
@beorn7 FYI