Store: Add tenant label to exported metrics

[jacobbaungard]

• I added CHANGELOG entry for this change.
• Change is not relevant to the end user.

Changes

This PR continues that work done in #6530 and #6595 to implement the "tenancy awareness in query path" proposal.

• Enhance relevant Store Gateway metrics with tenant information, in order to facilitate a per tenant view of store usage
• Added possibility to add HTTP headers to promclient.QueryOptions in order to create test cases where a tenant is set during querying
• Query tenant flags are no longer hidden as we now have some value in setting these
• Removed a few of debug log lines in bucket, as these are unnecessary as we now add the tenant information to the metrics

Verification

• Added e2e test for a subset of the tenant enhanced metrics

[saswatamcode]

cc: @douglascamata 🙂

[douglascamata]

Two very small comments, but overall looks awesome. 👍

[douglascamata]

I think there were some things removed accidentally here?

[jacobbaungard]

All of those were defined for convenience and initialized at init time. For example:

postingRequests = commonMetrics.requestTotal.WithLabelValues(cacheTypePostings) . Prior to adding the tenant label, that made sense, as one would simply need to do i.e postingRequests.Add(... when interacting with this value, as the label was "static".

However, now that we're adding a tenant label to all of these metrics, and as we don't know the tenant at init time, we need to type out the full thing every time we want to interact with this metric. So the above example has become:

requestTotal.WithLabelValues(cacheTypePostings, tenant).Add(...

Hopefully that makes sense.

[douglascamata]

I mean the removal of all the other counter above these, like postingRequests, seriesRequests, etc. Not sure if it's intended or accidental.

[jacobbaungard]

We're not removing anything here, just changing the way we reference these objects.

Before this change, we mapped all the metrics like this:

c.postingRequests = commonMetrics.requestTotal.WithLabelValues(cacheTypePostings)
c.seriesRequests = commonMetrics.requestTotal.WithLabelValues(cacheTypeSeries)
c.expandedPostingRequests = commonMetrics.requestTotal.WithLabelValues(cacheTypeExpandedPostings)

c.postingHits = commonMetrics.hitsTotal.WithLabelValues(cacheTypePostings)
c.seriesHits = commonMetrics.hitsTotal.WithLabelValues(cacheTypeSeries)
c.expandedPostingHits = commonMetrics.hitsTotal.WithLabelValues(cacheTypeExpandedPostings)

c.postingDataSizeBytes = commonMetrics.dataSizeBytes.WithLabelValues(cacheTypePostings)
c.seriesDataSizeBytes = commonMetrics.dataSizeBytes.WithLabelValues(cacheTypeSeries)
c.expandedPostingDataSizeBytes = commonMetrics.dataSizeBytes.WithLabelValues(cacheTypeExpandedPostings)

c.postingsFetchDuration = commonMetrics.fetchLatency.WithLabelValues(cacheTypePostings)
c.seriesFetchDuration = commonMetrics.fetchLatency.WithLabelValues(cacheTypeSeries)
c.expandedPostingsFetchDuration = commonMetrics.fetchLatency.WithLabelValues(cacheTypeExpandedPostings)

I.e there are 4 different metrics (3 before recent rebase), requestTotal, hitsTotal, dataSizeBytes, fetchLatency, each of these were initialized with 3 labels, resulting in 3x4 counters/observers, as seen in the mapping above.

With this PR, because we're introducing a "dynamic" label (i.e the tenant) we have to work of the base CounterVec/HistrogramVec objects, when we interact with these metrics, otherwise we cannot add the tenant label. So I removed the convenience mapping, and put the base objects there instead.

[douglascamata]

Any reason for not reusing the http.Header type here instead of map[string]string? This definition misses the fact that a header might have multiple values, in fact the best type for headers is map[string][]string.

[jacobbaungard]

Changed to use http.Header 👍

[saswatamcode]

Generally LGTM! Just a few small comments.

[saswatamcode]

I wonder...can we pass the tenant by context everywhere Instead of adding new a parameter? And add an extractor function in tenancy, that defaults to the default tenant?

Some methods don't have a ctx, but could add it for this, and make this tenant passing a consistent thing. Wdyt?

[jacobbaungard]

I've given this some thoughts as well. I agree, using ctx would make the function calls cleaner and less verbose. On the other hand, I like putting it explicitly as a function param, as it forces the caller to ensure the tenant information is available, whereas it could be easier to miss when using ctx.

In the end I think I prefer the current approach, but happy to change it if there are other opinions 👍

[douglascamata]

I'm +1 with @jacobbaungard. If the tenant parameter is key to changing the behavior or outcome of the function, I don't think it's a good idea to pass it through context. The official docs on contexts states: Use context Values only for request-scoped data that transits processes and APIs, not for passing optional parameters to functions..

[saswatamcode]

Sure, not a blocker. But tenant seems to me like request-scoped data that transits processes and APIs, which is why I suggested doing that. It also isn't optional anywhere right (we replace with default)?

[douglascamata]

Anything is request scoped data that transits in a system dealing with HTTP requests, no? 😂

[douglascamata]

But the tenant here is extremely important. If we put it in the context, we are basically hiding it and this is dangerous. If someone forgets to pass it, it will mess up all the per-tenant metrics and they will be wrong.

[douglascamata]

To me it doesn't seem a good idea to use context as a bag of objects because the change becomes smaller/easier in the codebase. In fact, it hurts discoverability because it hides important parameters from the function's signature.

[saswatamcode]

Fair, makes sense.

[saswatamcode]

Let's add a breaking :warning: here and a small note, as this might break any custom dashboards that people use.

[yeya24]

Is there a concern for cardinality? I don't think it is necessary to have tenant label for almost all metrics.

[saswatamcode]

@yeya24 I think this feature can be disabled by setting flags on querier/query-frontend, in case the user wants to avoid the extra cardinality. This seems like a useful feature to attribute query path resource cost to a particular tenant.

Maybe we can reconsider some of the metrics, when adding the tenant label later on @jacobbaungard?

[yeya24]

@saswatamcode Ah thanks for the clarification. I didn't notice that it can be turned off by setting flags.
But yeah I agree maybe we can revisit those labels at some point and see if they are really necessary.

[jacobbaungard]

Currently the tenant information is always propagated from the querier (default-tenant if nothing else is specified), and will be added the these metrics.

If there is a concern about these metrics having the tenant information added by default we could either:

• Don't forward the tenant header from the querier unless specifically set. I think this will add a fair bit of complexity to the code though.
• As suggested, take a closer look at which metrics specifically needs the tenant information. We'd probably want to ensure we do that before the next release in this case.

[jacobbaungard]

Although unless one actually starts querying data with different tenants, there will only be one label added (with the default-tenant value), so it probably shouldn't impact cardinality in this case.