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

[Feature]: spm additional query parameter required for prometheus #4221

Open
rbaumgar opened this issue Feb 7, 2023 · 9 comments · May be fixed by #6360
Open

[Feature]: spm additional query parameter required for prometheus #4221

rbaumgar opened this issue Feb 7, 2023 · 9 comments · May be fixed by #6360
Labels
enhancement good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement

Comments

@rbaumgar
Copy link

rbaumgar commented Feb 7, 2023

Requirement

I am using a global prometheus for spm where I need to add a specific query parameter to get data.
Currently no parameter is available to define an additional query parameter.
I only found --query.base-path which does not fit.

Problem

spm sends only e.g. "/api/v1/query?query=up", but I need to send "/api/v1/query?query=up&mykey=myvalue"
otherwise I don't get any data.

Proposal

please provide another parameter, eg. --query.additional-parameter.

Open questions

No response

@hkailantzis
Copy link

hkailantzis commented Jan 4, 2024

That would be great, as in my case we're using Thanos, and query parameter is a hard requirement for us (like the namespace), and can't alter this requirement, as I don't have access to that component. For the Bearer token I use the ---prometheus.token-file cli flag, which is basically a secret token generated by an appopriate SA (for all-in-one), and I'm mounting it using:

extraSecretMounts:
    - name: prometheus-token
      mountPath: /tls
      secretName: jaeger-all-in-one-token
      readOnly: true

this, plus RoleBinding of ClusterRole type:

apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
  name: jaeger
  namespace: default
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: ClusterRole
  name: <ClusterRoleName>
subjects:
- kind: ServiceAccount
  name: jaeger-all-in-one
  namespace: default

but I'm getting an error which I can't make anything out of it, so I'm assuming is because of the missing required namespace parameter..., but not 100% sure...

msg":"HTTP handler, Internal Server Error","error":"failed executing metrics query: bad_response: readObjectStart: expect { or n, but found B, error found in #1 byte of ...|Bad Request|..., bigger context ...|Bad Request. The request or configuration is malfor|..."

cc @rbaumgar

@rbaumgar
Copy link
Author

rbaumgar commented Jan 4, 2024

@hkailantzis I don't know what is required in your environment.
In OpenShift you have two ports available

@hkailantzis
Copy link

@rbaumgar, is the 9092 case actually, where a namespace parameter is required...which is not supported by jaeger SPM as the moment right ?

@rbaumgar
Copy link
Author

rbaumgar commented Jan 4, 2024

@hkailantzis correct! I don't want to use a cluster role for SPM...

@yurishkuro yurishkuro added help wanted Features that maintainers are willing to accept but do not have cycles to implement good first issue Good for beginners labels Jan 4, 2024
@prakrit55
Copy link
Contributor

@yurishkuro, I want to try it out

@akstron
Copy link
Contributor

akstron commented Dec 12, 2024

If I understand the issue correctly, we want to send extra paramters to the promethes endpoint, right? I checked the documentation but couldn't find any such server-side handling for such extra parameters. Are you trying to use a proxy in between?

Moreover, the current implementation uses prometheus client library instead of directly calling the http route, which only allows the paramters time and timeout as extra optional parameters mentioned in the doc:
https://prometheus.io/docs/prometheus/latest/querying/api/

@yurishkuro
Copy link
Member

@akstron we pass api.Client to the api.v1 which is used for executing the actual HTTP requests. The client is a very small API that is easy to wrap in a decorator if we want to add some extra parameters to the URL.

@akstron
Copy link
Contributor

akstron commented Dec 12, 2024

I see, that makes sense. I will start working on it. Thanks!

@yurishkuro
Copy link
Member

@akstron we should only do this in Jaeger v2 (i.e. don't both with v1 CLI flags for these headers).

@akstron akstron linked a pull request Dec 14, 2024 that will close this issue
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants