Metrics API Scaler support different format

[alileza]

Proposal

Extend metrics API scaler to support Prometheus format, or new scaler would be preferable?

I'm thinking something like

triggers:
- type: metrics-api
  metadata:
    targetValue: "8"
    url: "http://api:3232/api/v1/stats?format=prometheus"
    valueLocation: "go_memstats_mcache_inuse_bytes"

or instead of relying on ?format we can also rely on content-type: application/json will use gjson and content-type: text/plain will use Prometheus format.

unfortunately most of /metrics mostly use content-type: text/plain instead of explicitly saying it's Prometheus

Use-Case

Currently, metrics API Scaler only support JSON format which is pretty handy if the application exposes metrics using JSON.

but most application currently exposes metrics using Prometheus Metrics format, for example.

Anything else?

I can work on this, in case this feature would be something acceptable

[tomkerkhove]

format would be a good new field for this

[alileza]

oh right, yes new field would definitely better

[tomkerkhove]

Based on #2644 OpenMetrics is a very good idea for another format

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

[eldada]

Any plans on this?

[tomkerkhove]

No, are you willing to contribute this?

[eldada]

No, are you willing to contribute this?

Nope. Sorry. I don't. have the skill 😞

[tomkerkhove]

No worries!

[LuckyDogg]

hello guys , I am a new joiner which want to contribute to Keda and have read the contribute guide. I want to contribute on this issue. Could you give me some guide about this issue

[tomkerkhove]

@JorTurFer @zroubalik Can you help @LuckyDogg? Would be a nice addition

[JorTurFer]

Hey, sure 😄
You need to make the needed changes in this file: https://github.com/kedacore/keda/blob/main/pkg/scalers/metrics_api_scaler.go,
I guess that this is the function : https://github.com/kedacore/keda/blob/main/pkg/scalers/metrics_api_scaler.go#L226

Basically you need to add the needed changes in the scaler code, and cover it with [tests](https://github.com/kedacore/keda/blob/main/pkg/scalers/metrics_api_scaler_test.go. I guess that this feature should be covered by e2e test, but you can base it on the already existing test: https://github.com/kedacore/keda/tree/main/tests/scalers/metrics_api

If you have doubts building any component, you can check the build section, there is explained how to build and debug each component

[LuckyDogg]

got it. many thanks. I will work on this issue
/assign

[brybacki]

Hey @LuckyDogg this is really good ISSUE, are you working on this?

[brybacki]

I have hacked something quickly in my free time:
https://github.com/brybacki/keda/tree/metrics-api-prom
I suppose I can turn it into a PR

I did some experiments on kubernetes cluster and it works.

[JorTurFer]

I think that nobody was working on this 😄
If you open a PR it'd be nice

[sarunasandr]

Hello @brybacki. Are you planning to turn it into a PR? I am currently experiencing issues related to this, it would be nice to have it solved.

[fira42073]

Hi. Hope you are well.
You can assign this one to me, I'm planning to contribute.
I'll text with questions to clarify the requirements further.
Thanks!

[fira42073]

Hey! Hope y'all are doing good in this year!

I see that we are using gjson as a tool to get value from path.

gjson works only for json, which means we would have to get a library for each of the formats we use or create a custom solution.

I kinda have a solution (in context of yaml and xml).
I'm not sure how justified it is, but in the PR that I have attached #5347 you can see that I've created a function called GetValueByPath and it essentially walks any map[string]interface{} data structure to get that value. Do we want to use the same approach for each of the types we are going to support? Bear in mind that XML does not always parse into a go struct, because of attributes etc. and there is a library https://github.com/antchfx/xpath that provides much more functionality.
I were not able to find such a library for yaml.

Also, which formats do we need to support? I presume that clients might have SOAP API's that use xml, but I'm not so sure about yaml.

Regardless of the solution, it shall be very well-documented as the usecase is not that simple C:

Thanks in advance.

[JorTurFer]

I've quick reviewed your PR and it looks really nice and I like the approach you've taken tbh.
@zroubalik @tomkerkhove ?

[zroubalik]

Let me get to this in a few days, thanks for the proposal.

@wozniakjan FYI

[smojum]

Hey! Hope y'all are doing good in this year!

I see that we are using gjson as a tool to get value from path.

gjson works only for json, which means we would have to get a library for each of the formats we use or create a custom solution.

I kinda have a solution (in context of yaml and xml). I'm not sure how justified it is, but in the PR that I have attached #5347 you can see that I've created a function called GetValueByPath and it essentially walks any map[string]interface{} data structure to get that value. Do we want to use the same approach for each of the types we are going to support? Bear in mind that XML does not always parse into a go struct, because of attributes etc. and there is a library https://github.com/antchfx/xpath that provides much more functionality. I were not able to find such a library for yaml.

Also, which formats do we need to support? I presume that clients might have SOAP API's that use xml, but I'm not so sure about yaml.

Regardless of the solution, it shall be very well-documented as the usecase is not that simple C:

Thanks in advance.

I'm currently seeking support for the Prometheus format. I noticed that @brybacki has developed something similar in a fork. Is there any possibility of incorporating this into the main branch? I'm considering the creation of a custom scalar due to the urgency of my requirement for Prometheus format. However, before I proceed with the custom scalar, I wanted to explore the option of advancing this feature in the main project. I am willing to take on the responsibility for contributing to this development to ensure its integration

[zroubalik]

@smojum can you please give us more detail about the proposal? Prometheus Scaler is not suitable for you?

[smojum]

@smojum can you please give us more detail about the proposal? Prometheus Scaler is not suitable for you?

The Prometheus Scalar isn't an appropriate solution for us because the Prometheus instance in our cluster operates in agent mode. I believe this mode doesn't support querying. Although it's possible to retrieve metrics from a remote Prometheus server where the data is stored, we want to avoid opening an external connection from our cluster for security reasons. However, we do have metrics available at the service's metrics endpoint. The challenge here is that these metrics are in Prometheus format, as opposed to the JSON format expected by the Metric API scalar.

[JorTurFer]

we do have metrics available at the service's metrics endpoint

Are them aggregated somehow by the prometheus agent? I mean, if you have 5 instances of your API exposing prometheus metrics, despite KEDA can read and understand prometheus format, KEDA won't query and aggregate all the instances, it just query one of them and will scale based on it, which probably isn't what you expect (and we aren't taking into account the option of doing queries, only raw metrics would be available).
Does it still work for you?

[smojum]

Does it still work for you?

Absolutely valid point. This metrics is something which we are getting from database (and not something which is pod/instance specific). so the value will be consistent irrespective of instance from where we are retrieving the data.

[zroubalik]

In general, I am open to exteding the scaler to support additional types. @smojum could you please draft here, what would be the structure and new fields added to the scaler (write an example config).

[smojum]

• Option 1:

  triggers:
    - type: metrics-api
      metadata:
        url: http://keda-metric-app-svc.default.svc.cluster.local/prom-metric
        targetValue: "30"  # Threshold for scaling
        format: prometheus # picked this value from the PR (enum)
        promMetricLocation: "metric" # name of the metrics e.g. rpc_duration_seconds{quantile="0.99"} including the dimension
        promMetricType: "summary" # possible values counter, summary, histogram, gauge. I am thinking this will probably help in parsing the value, may keep as optional

I think we already in the process of adding format. Additional 2 fields are promMetricName and promMetricType. If we need to optimize, we can use existing valueLocation instead of promMetricLocation, and get rid of promMetricType. (I honestly don't think it is needed for parsing, but added just in case.

• Option 2

  triggers:
    - type: metrics-api
      metadata:
        url: http://keda-metric-app-svc.default.svc.cluster.local/prom-metric
        targetValue: "30"  # Threshold for scaling
        format: prometheus # picked this value from the PR (enum)
        valueLocation: "rpc_duration_seconds{quantile="0.99"}" # name of the metrics

[zroubalik]

Hi, I prefer Option 1 Option 2.

Thoughts @kedacore/keda-maintainers @wozniakjan ?

// EDIT: corrected preference

[wozniakjan]

I have a weak preference towards option 2, I feel like additional options dedicated to a specific metric format should be added when there is a direct and known use for them.

Also having promMetricLocation which behaves the same way valueLocation would, but is only for prometheus, might make the configuration a bit scattered and degrade the UX.

[zroubalik]

I have a weak preference towards option 2, I feel like additional options dedicated to a specific metric format should be added when there is a direct and known use for them.

Also having promMetricLocation which behaves the same way valueLocation would, but is only for prometheus, might make the configuration a bit scattered and degrade the UX.

OMG, I agree absolutely with those points, exactly my thoughts. I meant to write that my preference is option 2 but wrote 1 instead, don't know why 🤦‍♂️ 😄

[smojum]

If folks are okay, I can create a PR for Option 2. Please let me know any other thoughts.

[fira42073]

Hello. Hope you all are doing well. I've added some changes to the PR to support Prometheus format. I wonder which formats need to be supported as well.
Please review if the functionality is adequate and let me know if you have any comments: #5347

[fira42073]

Regarding my last comment, I need to add adequate tests for GetValueFromResponse, and if all else is good, then I'll go through checklist, add docs etc. and it's good to be merged afterwards. Please review my PR and if everything looks fine, I'll proceed with it.

Thanks in advance!
https://github.com/kedacore/keda/pull/5347/files
@zroubalik @tomkerkhove

[abinatarajan]

any update on this?

[fira42073]

Hello, @abinatarajan
I'm waiting on review.
I would appreciate any comments:
#5347

[fira42073]

@zroubalik @wozniakjan
Hello! I hope you are well.
Documentation for this change is not merged yet kedacore/keda-docs#1325

[JorTurFer]

my bad, I merged the feature without reviewing the docs :(