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

Custom metric selector invalid label value #364

Closed
pytimer opened this issue Jan 27, 2021 · 8 comments
Closed

Custom metric selector invalid label value #364

pytimer opened this issue Jan 27, 2021 · 8 comments
Labels
lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.

Comments

@pytimer
Copy link

pytimer commented Jan 27, 2021

Hi,
I use custom metrics hpa, my application expose http_server_requests_seconds_count metric, and this metric like below:

Element Value
http_server_requests_seconds_count{application="web",endpoint="web-port",instance="192.168.10.112:8080",job="demo",method="POST",namespace="dev",pod="dev-demo-6c45bd669c-w47sj",service="demo-svc",status="200",uri="/v1/login"} 2
http_server_requests_seconds_count{application="web",endpoint="web-port",instance="192.168.10.112:8080",job="demo",method="GET",namespace="dev",pod="dev-demo-6c45bd669c-w47sj",service="demo-svc",status="200",uri="/v1/task"} 5

I want my application according the uri of metric to scale up/down. I can use selector to filter, but label uri value have / character, i use this value as filter, hpa have the warning: invalid label value: "/company/security/v1/account/login": at key: "uri": a valid label must be an empty string or consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyValue', or 'my_value', or '12345', regex used for validation is '(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?').

How to do with selector, or use another way?

My hpa:

apiVersion: autoscaling/v2beta2
kind: HorizontalPodAutoscaler
metadata:
  name: dev-demo
  namespace: dev
spec:
  maxReplicas: 2
  metrics:
  - pods:
      metric:
        name: http_server_requests_seconds_count
        selector:
          matchExpressions:
          - key: uri
            operator: In
            values:
            - /v1/task
      target:
        averageValue: "20"
        type: AverageValue
    type: Pods
  minReplicas: 1
  scaleTargetRef:
    apiVersion: apps/v1
    kind: Deployment
    name: dev-demo
@dgrisonnet
Copy link
Member

Hi @pytimer,

A possible workaround would be to use external metrics and put your label selector directly inside of the seriesQuery of your prometheus-adapter configuration. Given your hpa configuration you would have something like that:

seriesQuery: 'http_server_requests_seconds_count{namespace!="", pod!="", uri="/v1/task"}'

Note that moving to external metrics is required for this workaround as the labelSelectors given in the seriesQuery are currently filtered for custom metrics.

@pytimer
Copy link
Author

pytimer commented Feb 2, 2021

Hi @pytimer,

A possible workaround would be to use external metrics and put your label selector directly inside of the seriesQuery of your prometheus-adapter configuration. Given your hpa configuration you would have something like that:

seriesQuery: 'http_server_requests_seconds_count{namespace!="", pod!="", uri="/v1/task"}'

Note that moving to external metrics is required for this workaround as the labelSelectors given in the seriesQuery are currently filtered for custom metrics.

Thanks @dgrisonnet . But if i setting seriesQuery of parometheus-adapter configuration, i need to write multiple rules according to uri. I see the external metrics only support standard kubernetes label selector, not support /v1/task.

In my case, the application output many metrics with uri label, if setting seriesQuery rules, it is difficult for developers.

@dgrisonnet
Copy link
Member

From the look of it, I don't see any other option for your use case. The best would be to be able to modify the HPA to scale based on the uri label but as you mentioned it doesn't support non-standard Kubernetes labels.

That being said, it might be worth following up on this issue in the Kubernetes repository as I wouldn't expect external metrics' label selector to be tied to Kubernetes standard. IMO, it should be used to select metrics labels, not k8s labels. Thus, it shouldn't abide to Kubernetes' label standard.

@s-urbaniak do you by any chance have other suggestions?

@pytimer
Copy link
Author

pytimer commented Feb 9, 2021

That being said, it might be worth following up on this issue in the Kubernetes repository as I wouldn't expect external metrics' label selector to be tied to Kubernetes standard. IMO, it should be used to select metrics labels, not k8s labels. Thus, it shouldn't abide to Kubernetes' label standard.

If the HPA support metrics labels, it maybe involve in multiple repository, such as custom-metrics-apiserver, metrics, and also need to consider how to be compatible with existing interface.

@s-urbaniak
Copy link
Contributor

s-urbaniak commented Feb 9, 2021

Another method that comes into my mind is relabeling during ingestion: https://prometheus.io/docs/prometheus/latest/configuration/configuration/#relabel_config

You could write a regex that replaces / with - making the selector happy.

However, a word of warning regarding using uris as label values. It very often can be a cause of cardinality explosion, especially if the amount of know URIs has a high upper bound or even worse if your metrics exposes all uri values, even unknown ones. Maybe rather use endpoint labels or simply wrap your middleware handlers with concrete metrics and call them something like task_requests_seconds_count, see prometheus/client_golang#491 for a good discussion about potential dangers with using raw paths or uris as label values.

@pytimer
Copy link
Author

pytimer commented Feb 14, 2021

Thanks @s-urbaniak . I know use the uri on the potential effects on prometheus, i replaces / with - to solve this problem.

But i think prometheus-adapter is best to support any format label value instead of k8s standard label. It maybe a bit wrong to propose this feature here.

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 15, 2021
@dgrisonnet
Copy link
Member

Let's keep track of this issue in #323.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.
Projects
None yet
Development

No branches or pull requests

5 participants