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

Add mimir support with org id header #1928

Merged
merged 6 commits into from Jul 26, 2023
Merged

Conversation

melnikovio
Copy link
Contributor

@melnikovio melnikovio commented May 18, 2023

What does this PR change?

Add required by Mimir header X-Scope-OrgID in http Prometheus client.

To use Mimir instead of Prometheus add variable:
"PROMETHEUS_HEADER_X_SCOPE_ORGID": "my-cluster-name"

Then set Prometheus URL to Mimir endpoint:
"PROMETHEUS_SERVER_ENDPOINT": "http://mimir-url/prometheus/"

Does this PR relate to any other PRs?

No

How will this PR impact users?

Adds ability to use Mimir storage instead of Prometheus

Does this PR address any GitHub or Zendesk issues?

No

How was this PR tested?

Manual

Does this PR require changes to documentation?

Yes

Have you labeled this PR and its corresponding Issue as "next release" if it should be part of the next Opencost release? If not, why not?

Required release

Signed-off-by: Ilya Melnikov <me@melnikov.io>
@melnikovio melnikovio marked this pull request as ready for review May 18, 2023 05:48
@melnikovio melnikovio changed the title feat: add mimir support with org id header Add mimir support with org id header May 18, 2023
@Pokom
Copy link
Contributor

Pokom commented May 24, 2023

Hey @melnikovio, love this change! Out of curiosity, how are you running Mimir(on-prem vs Grafana Cloud)? What are you seeing as a response from Mimir?

My only suggestion here is that we should create a separate client similar to what we do for Thanos and instantiate it only if we detect the Mimir Org ID is set. It doesn't feel correct to update all of the other clients for a Mimir specific implementation detail.

@melnikovio
Copy link
Contributor Author

Hey @melnikovio, love this change! Out of curiosity, how are you running Mimir(on-prem vs Grafana Cloud)? What are you seeing as a response from Mimir?

Thank you @Pokom!
We are running Mimir on-prem.
Mimir has an API that tries very hard to be like Prometheus. Therefore, by changing the header and specifying the mimir address, the application works without problems, performing the same requests with calculations (avg) for UI.
There is only one warning that I saw about missing status config:
2023-05-24T16:34:52.707154+03:00 INF No valid prometheus config file at https://mimir/prometheus/. Error: client_error: client error: 404 . Troubleshooting help available at: http://docs.kubecost.com/custom-prom#troubleshoot. Ignore if using cortex/thanos here.

My only suggestion here is that we should create a separate client similar to what we do for Thanos and instantiate it only if we detect the Mimir Org ID is set. It doesn't feel correct to update all of the other clients for a Mimir specific implementation detail.

At the first time I made a separate client. But it will literally differ by exactly one line with the Header. And I thought it was redundant. If you think new client is better - I'll change it.

@WoozyMasta
Copy link

WoozyMasta commented May 24, 2023

@Pokom
X-Scope-OrgID is not only a header for Mimir, it originally appeared in Cortex-Tenant
You are not going to create two identical clients separately for Mimir or Cortex or Victoriametrics

https://cortexmetrics.io/docs/guides/auth/
https://docs.victoriametrics.com/vmauth.html

What difference does it make whether it is a cloud installation or a local one. To work with multi-tenancy, you need to pass a header.

If you don't like the implementation with a separate Mimir client, let's globally add the ability to add headers to all clients. Let the backend decide whether it needs this header or not.

@mattray
Copy link
Collaborator

mattray commented May 24, 2023

I agree with @Pokom, it seems that exposing X-Scope-OrgID with the default for Prometheus and letting different implementations override it seems straightforward. Kubecost uses Thanos, so they probably have opinions too.

@AjayTripathy want to weigh in?

@WoozyMasta
Copy link

@Pokom , @mattray
hi, any thoughts on this PR?

for those who also need the X-Scope-OrgID header, I will give a hint how to get around this, I add another proxy to the already existing Nginx in opencost-ui via ConfigMap, which adds the X-Scope-OrgID header

upstream prometheus {
  # Mimir/Cortex endpoint
  server mimir-query-frontend.monitoring.svc:8080;
}

server {
  listen 9091;
  listen [::]:9091;
  resolver 127.0.0.1 valid=5s;

  location / {
      proxy_pass        http://prometheus/;

      # X-Scope-OrgID with tenant names
      proxy_set_header  X-Scope-OrgID   my-first-tenant|my-second-tenant;
      proxy_set_header  X-Real-IP       $remote_addr;
      proxy_set_header  X-Forwarded-For $proxy_add_x_forwarded_for;

      proxy_connect_timeout       180;
      proxy_send_timeout          180;
      proxy_read_timeout          180;

      proxy_redirect    off;
    }
}

and use this prometheus address

PROMETHEUS_SERVER_ENDPOINT=http://127.0.0.1:9091/prometheus

I would like to do it without workarounds

@@ -1535,6 +1535,7 @@ func Initialize(additionalConfigWatchers ...*watcher.ConfigMapWatcher) *Accesses
},
QueryConcurrency: queryConcurrency,
QueryLogFile: "",
MimirHeaderOrgId: env.GetMimirHeaderOrgId(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we just rename this field to something like xScopeOrgIDHeader to make it a bit more generic, especially if its not specific to Mimir?

Copy link
Contributor Author

@melnikovio melnikovio Jun 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -92,6 +92,8 @@ const (
PrometheusRetryOnRateLimitMaxRetriesEnvVar = "PROMETHEUS_RETRY_ON_RATE_LIMIT_MAX_RETRIES"
PrometheusRetryOnRateLimitDefaultWaitEnvVar = "PROMETHEUS_RETRY_ON_RATE_LIMIT_DEFAULT_WAIT"

MimirHeaderOrgIdEnvVar = "MIMIR_HEADER_ORG_ID"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same with making this generic?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Now variable is: PROMETHEUS_HEADER_X_SCOPE_ORGID

Copy link
Contributor

@AjayTripathy AjayTripathy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late responses here @melnikovio -- I would support this change would just want to make this generic so you can pass any X-SCOPE-ORG-ID header. That seem like a fair compromise?

ilya.melnikov added 3 commits June 2, 2023 08:44
Signed-off-by: ilya.melnikov <ilya.melnikov@idp.zyfra.com>
Signed-off-by: ilya.melnikov <ilya.melnikov@idp.zyfra.com>
Signed-off-by: ilya.melnikov <ilya.melnikov@idp.zyfra.com>
@melnikovio
Copy link
Contributor Author

@AjayTripathy pls review pr

@graemechristie
Copy link

Can this be reviewed @AjayTripathy ? We are also wanting to use opencost with Mimir (self hosted).

@mehulpurohit97
Copy link

Hi @AjayTripathy , I hope this message finds you well. I noticed this pull request and was wondering about its status. In our project, we are also in need of mimir support.

@AjayTripathy AjayTripathy removed their request for review July 26, 2023 16:53
@AjayTripathy
Copy link
Contributor

@cliffcolvin can we get this reviewed by the core team?

@nikovacevic nikovacevic dismissed AjayTripathy’s stale review July 26, 2023 18:11

The changes were made, and he asked for someone else to review.

@nikovacevic nikovacevic merged commit dfe384a into opencost:develop Jul 26, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants