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

Joining different labels #2204

Open
fabxc opened this Issue Nov 18, 2016 · 18 comments

Comments

Projects
None yet
10 participants
@fabxc
Copy link
Member

fabxc commented Nov 18, 2016

If we are monitoring complex systems, we might get metrics on different connected entities from different metric providers. The label naming might not be 100% aligned, i.e. in Kubernetes we get container metrics with a pod_name label from cAdvisor and metrics with a pod label from kube-state-metrics.
The values are the same pod identifying names. However, we have to use label_replace() to be able to join them properly, which is always somewhat awkward and hard to decipher.

I think this will be a common case across many systems and first-class support to join such labels could be beneficial. I imagine a syntax along the lines of:

container_last_seen and on(pod_name=pod) kube_pod_info

Where the names left and right of = refer to the matched on labels of the left and right metrics respectively.
I'd hope it to be easy enough to implement in our parser and think it's generally an rather obvious syntax for the semantics.

I know that's not a trivial extension to PromQL, so this is just to start a discussion.

@brian-brazil

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Nov 18, 2016

The correct answer here is to fix the exporter, I consider label_replace and what you propose here only as stop-gaps until that can happen.

@fabxc

This comment has been minimized.

Copy link
Member Author

fabxc commented Nov 18, 2016

I figured that would be the go-to response.

I agree with that when it comes to labels containing concatenated information we have to split before joining etc. – but it's just an unrealistic expectation to align 3+ components written by different teams. Especially if those have different stability concerns and won't simply accept breaking their metrics used by thousands of people.
Neither is it to have all those users use label_replace in dozens of alerts and especially not in ad-hoc queries.

I think is a another case where we have to carefully distinguish between what's acceptable and feasible in open source vs a single huge organisation.

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Nov 18, 2016

There's also the 3rd option of metric_relabel_configs.

I'm used to it being really uncommon for you to want to join data across jobs, let alone with different labels.

@fabxc

This comment has been minimized.

Copy link
Member Author

fabxc commented Nov 18, 2016

I see it to be really common actually – e.g. cAdvisor exports container metrics labeled with pod name. Kubelet exports pod info metrics (at least I think that's the best source, issue pending on that), kube-state-metrics exporting metrics on abstract objects like deployments, replica sets, etc.
I'd generally take all those together and assemble some recording rules providing me with comprehensive metrics on resource usage by a deployment, through pods belonging to them, and containers belonging to those.

These would mostly be recording rules, yes – they'd be fairly static and label_replace would be okay. However, in the end if I want to drill down I'd have to use that ad-hoc again and again.
I'm not a fan of metric_relabel_configs as they alter the state of well-documented metrics. Prometheus doesn't have the usability facilities to make up for that, i.e. proper query-time completion on potential labels per metric and their values, ...
Also, metric relabeling is not exactly cheap.

I know it's not an ideal state of the world thing to have different label names. But I consider it a real-world concern. Would this addition have any weird edge-case semantics or be confusing?

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Nov 18, 2016

In the systems I'm used to, the equivalent pod data and instance data would almost never be joined - it wasn't needed in practice to actually link things together end to end to do the alerting you wanted. k8 is a more complicated system though.

This does sound like something sig-instrumentation should be standardising.

Would this addition have any weird edge-case semantics or be confusing?

I've only been thinking on it a few minutes, but I see potential issues around e.g. without - how do you spell it then? What are the semantics?

Which of the two labels does the output metric have? Do we break the commutivity we currently have? How does this interact with group_left?

Label matching is already confusing to users, so I'm wary of adding more modifiers.

The only place I've ever seen anything like this is for alertmanager inhibits, for when you want to use an alert another team has setup for this purpose but you called your zone/cluster/datacenter label something else.

A 4th option is to add label_copy, as label_replace is unwieldy for that simple case.

@fabxc

This comment has been minimized.

Copy link
Member Author

fabxc commented Nov 18, 2016

Yes, I'm trying to think about standardisation of k8s metrics but see exactly those differing requirements of metric stability.

As it would be an addition, I don't think we'd break anyone. This would only affect on().
We'd indeed have to pick one side, probably left, so it would break commutativity, which isn't nice. I agree that matching is already confusing to many and added complexity is not great.

label_copy might be a step that's easier to understand for sure.

We might want to add that and see over a few weeks how far standardisation efforts get us.

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Nov 18, 2016

This would only affect on().

We'd need it with ignoring too, as in principle you should be favouring ignoring over on to produce generic shareable rules. The alternative would involve this descending into a 2nd form of group_left where we keep all the labels from one side.

label_copy is in #959

@beorn7

This comment has been minimized.

Copy link
Member

beorn7 commented Nov 18, 2016

On 18 November 2016 at 12:57, Fabian Reinartz notifications@github.com
wrote:

However, in the end if I want to drill down I'd have to use that ad-hoc
again and again.
[...] Prometheus doesn't have the usability facilities to make up for
that, i.e. proper query-time completion on potential labels per metric and
their values, ...

As a side note: I think we should really improve on the auto completion in
the expression browser. We were quite far already, and essentially gave up
because of a trivial usability quirk. If I hadn't so many other things on
my plate, I'd work on it, despite having no real expertise in UIs.

And if proper auto completion would solve the issue discussed here, I'd
prefer it over making PromQL more complex.

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

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Nov 18, 2016

Auto-completion is a good thing, but I don't see it helping much with the fundamental issue here.

@fabxc

This comment has been minimized.

Copy link
Member Author

fabxc commented Nov 18, 2016

Let's say it would alleviate some of the pain of rarely having a clue what labels a given metric has and not being able to see its help text. and hence relying more strongly on application's documentation of its metrics. That return might make people more reluctant to break their metrics.

OTOH, this only helps Prometheus, which is not the only system applications export metrics for.

@davidquarles

This comment has been minimized.

Copy link

davidquarles commented Apr 3, 2018

@fabxc @brian-brazil among the options listed, which do you recommend? below is what i'm currently doing to enable the exact workflow @fabxc mentioned, which in our case supports dashboards, alerts, and ad-hoc queries. I'm curious what the footprint of and tradeoffs between the various options are.

metric_relabel_configs:

# supports joins to node-exporter metrics
- source_labels: [kubernetes_name, node]
  regex: 'kube-state-metrics;(.+)'
  target_label: instance

# supports joins to cadvisor metrics
- source_labels: [kubernetes_name, pod]
  regex: 'kube-state-metrics;(.+)'
  target_label: pod_name

# supports label-based, workload-level queries
- action: labelmap
  regex: label(_k8s|)_app
  replacement: app

# supports owner-based, workload-level queries
- source_labels: [kubernetes_name, __name__, owner_kind, owner_name]
  regex: 'kube-state-metrics;kube_pod_owner;(.+);(.+)'
  replacement: '$1/$2'
  target_label: owner

# supports owner-based queries targeting the parent deployment
- source_labels: [kubernetes_name, __name__, owner_kind, owner_name]
  regex: 'kube-state-metrics;kube_pod_owner;ReplicaSet;(.+)-(.+)'
  replacement: 'Deployment/$1'
  target_label: owner
groups:
- name: pod-info
  rules:
  - record: kube_pod_info_ex
    expr: |
      # fall back to app label for static/orphaned pods
      label_replace(
        kube_pod_labels
        * on(namespace, pod) group_left(host_ip, node, pod_ip)
        kube_pod_info
        * on(namespace, pod) group_left (owner_is_controller, owner, owner_kind, owner_name)
        kube_pod_owner{owner_is_controller="<none>"},
        "owner", "$0", "app", ".*"
      )
      or on (namespace, pod)(
        kube_pod_labels
        * on(namespace, pod) group_left(host_ip, node, pod_ip)
        kube_pod_info
        * on(namespace, pod) group_left (owner_is_controller, owner, owner_kind, owner_name)
        kube_pod_owner{owner_is_controller="true"}
      )
- name: pod-aggregates
  rules:
  - record: pod_memory_usage_bytes
    expr: |
      kube_pod_info_ex
      * on (namespace, pod_name) group_left
      sum by (namespace, pod_name)(container_memory_usage_bytes)
  - record: pod_spec_memory_limit_bytes
    expr: |
      kube_pod_info_ex
      * on (namespace, pod_name) group_left
      sum by (namespace, pod_name)(container_spec_memory_limit_bytes)
  - record: pod_spec_memory_request_bytes
    expr: |
      kube_pod_info_ex
      * on (namespace, pod_name) group_left
      sum by (namespace, pod_name)(kube_pod_container_resource_requests_memory_bytes)
      or on (namespace, pod_name)
      kube_pod_info_ex > bool 1
  # other resources...
- name: workload-aggregates
  # ...
- name: node-aggregates
  # ...
@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Apr 3, 2018

Fixing the source data is the best option, but failing that metric_relabel_configs is best.

@chancez

This comment has been minimized.

Copy link

chancez commented Jul 24, 2018

I'd be super happy if a label_copy was added, that's all that's really needed here, and potentially a label_remove, though that's not as necessary for the case of joining.

@gaord

This comment has been minimized.

Copy link

gaord commented Aug 16, 2018

yes, label names are different across metrics but means the same thing, that sounds like an often scenario of integration from various sources of metrics. Hopefully they could be joined with different names.

@tangxuye

This comment has been minimized.

Copy link

tangxuye commented Aug 30, 2018

I wonder if some scenes that we need add some labels to the metrics of the third-exporter.For example, the node_exporter,if I want to add one or more labels to the metrics.What should I do?

@gjcarneiro

This comment has been minimized.

Copy link

gjcarneiro commented Nov 26, 2018

I have the same problem, I need to join kube_pod_labels, which has a pod label containing the pod name, with the cAdvisor metrics such as container_cpu_usage_seconds_total, which has a pod_name label, same value but different label name.

@Sennahoi

This comment has been minimized.

Copy link

Sennahoi commented Jan 28, 2019

I would love the requested feature as well.

@FakeEmperor

This comment has been minimized.

Copy link

FakeEmperor commented Apr 8, 2019

I would also like to see this (being able to do on() and ignoring() with matching different labels) implemented.

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