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

Doing group_left() multiplication (left cardinality > right side) while also wanting to add a 1:1 matching label from the right side #1503

Closed
jjo opened this Issue Mar 24, 2016 · 13 comments

Comments

Projects
None yet
5 participants
@jjo
Copy link

jjo commented Mar 24, 2016

I'd like to compose a query to 'add' nodename label (as provided by
node_uname_info metric from node_exporter) by matching it against
instance - this is easy to do when there's no extra cardinality than
instance itself on the left-side - e.g. this works ok:

node_load1 * on (instance) group_right(nodename) node_uname_info

But then left-side has 'more cardinality' (more labels than just instance,
in this case I want to 'keep' device), I can't find a way to do it.

Take below example:

  • rate(node_disk_reads_completed[1m])
  {device="sda",instance="10.0.0.1:9100",job="node-exporter"} 1.0
  {device="sdb",instance="10.0.0.2:9100",job="node-exporter"} 2.0
  {device="sdc",instance="10.0.0.3:9100",job="node-exporter"} 3.0
  • node_uname_info
  node_uname_info{instance="10.0.0.1:9100",job="node-exporter",nodename="alice",...} 1
  node_uname_info{instance="10.0.0.2:9100",job="node-exporter",nodename="bob",...} 1
  node_uname_info{instance="10.0.0.3:9100",job="node-exporter",nodename="claire",...} 1

=> below expression is valid but obviously doesn't keep nodename from
node_uname_info (i.e. a useless/no-op in this case):

rate(node_disk_reads_completed[1m]) * on (instance) group_left(device) node_uname_info

=> then the one below is invalid:
(while I'd love it to be ok ;-), knowing the right side would be a 1:1 mapping):

rate(node_disk_reads_completed[1m]) * on (instance) group_left(device) group_right(nodename) node_uname_info

Anyway to do above, with current operators support ?

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Mar 24, 2016

This is not presently possible, it's on my todo list to figure out what semantics change is needed on group_left to enable this (hopefully without adding more keywords).

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Apr 6, 2016

I've been thinking on this a bit, and think I've got a solution.

We want something that works for both the node_cpu case where there's an extra label on the many side, and also in the machine role case where we want to copy over labels from the one side. In both cases we want to keep all labels from the many side (particularly if you're using without, we need this to work when there are unknown job-level labels that we should be preserving).

What I propose is two fold:

First just as for by there's without, I propose ignoring to go with on. This will do matching for binops ignoring certain labels.

Secondly I propose that the labels listed in the group_ modifier be copied from the one side to the many side. It will be valid to specify no labels, to cater for the node_cpu case. Currently they're copied from the many side, so this break all existing users of group_.

The node_cpu case would look like rate(node_cpu[5m]) / ignoring (mode) group_left sum without (mode)(rate(node_cpu[5m])).

The machine role case would look like my_var * ignoring (scraped_labels_on_lhs) group_left(labels_to_copy_from_rhs) node_role

@grobie

This comment has been minimized.

Copy link
Member

grobie commented Apr 6, 2016

So to clarify: the labels from the many side will always be used, in addition the labels specified in group_* modified will be copied over?

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Apr 6, 2016

Yes.

I don't think there's a sane use with the on modifier (other than copying a label from one-to-many when it's really one-to-one), so we may wish to make that an invalid syntax to avoid confusion.

@beorn7

This comment has been minimized.

Copy link
Member

beorn7 commented Apr 12, 2016

I tried to wrap my head around this, and so far I think this looks good.

Any other opinions?

Should we turn this into a GH issue to kickstart implementation?

Björn Rabenstein, Engineer
http://soundcloud.com/brabenstein

SoundCloud Ltd. | Rheinsberger Str. 76/77, 10115 Berlin, Germany
Managing Director: Alexander Ljung | Incorporated in England & Wales
with Company No. 6343600 | Local Branch Office | AG Charlottenburg |
HRB 110657B

@fabxc

This comment has been minimized.

Copy link
Member

fabxc commented Apr 12, 2016

It's a GH issue already :)

@fabxc

This comment has been minimized.

Copy link
Member

fabxc commented Apr 12, 2016

This seems to work 👍

@beorn7

This comment has been minimized.

Copy link
Member

beorn7 commented Apr 12, 2016

👊 Yeah, failed to recognize it's on GH already...

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Apr 21, 2016

I've implemented this now in #1571. I've broken it into stages so you can see the rules at various stages, what must change and how rules can get a bit cleaner.

There's a corner case where someone wants to copy a label over from a metric with no common labels. If that was workable with ignoring, then on could be removed for one-to-many as it'd support no additional use cases. However that doesn't seem to be possible.

I then looked at tweaking on in the one-to-many case to keep all labels from the many side, and copy from the one. This turns out to be sufficient (and is potentially a little easier to use in some cases, we'll have to see how it's used in practice).

Where this doesn't feel right is that in one-to-one cases on is only returning the listed labels, whereas in one-to-many it's returning all the labels on one side. ignoring does the same, but doesn't feel as bad to me as the matching labels vs. returned labels distinction comes up less.

Our two options then are to either not support the corner case and drop on; or to have different matching vs. returned semantics for one-to-many and one-to-one be a thing users need to really understand. I'm tending towards the latter as the distinction is going to come up anyway, and thus accepting the full changes proposed in the PR.

The one disadvantage of this is that it scuppers any hope of a graceful transition for users, as we're changing the existing on semantics for one-to-many. This will break all users using the machine role approach, but it's likely resolvable via sed as the name of the machine role variable is going to be well known.

ignoring is independently useful, and we should add it in either case.

@beorn7

This comment has been minimized.

Copy link
Member

beorn7 commented Apr 24, 2016

Where this doesn't feel right is that in one-to-one cases on is only returning the listed labels, whereas in one-to-many it's returning all the labels on one side. ignoring does the same, but doesn't feel as bad to me as the matching labels vs. returned labels distinction comes up less.

I think the two different semantics are acceptable as they go along with a group_{left,right} modifier, so the change is explicitly marked and doesn't happen as surprise because of a changed context.

@beorn7

This comment has been minimized.

Copy link
Member

beorn7 commented Apr 24, 2016

We could even rationalize it as follows: The default semantics is to keep all labels from the grouping side. If there is no grouping side defined, only the matching labels can be kept to retain symmetry.

@beorn7 beorn7 referenced this issue Apr 24, 2016

Merged

Fix for #1503 #1571

@fabxc fabxc added this to the v1.0.0 milestone Apr 25, 2016

brian-brazil added a commit that referenced this issue Apr 26, 2016

@grobie grobie closed this Apr 27, 2016

@jjo

This comment has been minimized.

Copy link
Author

jjo commented Apr 27, 2016

Thanks @brian-brazil for tackling this :)

@lock

This comment has been minimized.

Copy link

lock bot commented Mar 24, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators Mar 24, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.