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

Implement tenant federation for metadata querier endpoint #2467

Merged
merged 4 commits into from
Jul 20, 2022

Conversation

56quarters
Copy link
Contributor

@56quarters 56quarters commented Jul 19, 2022

Signed-off-by: Nick Pillitteri nick.pillitteri@grafana.com

What this PR does

Allow multiple tenant IDs to be passed to the /api/v1/metadata
endpoint in the same way we support for query and exemplar endpoints.

For other endpoints, the tenant ID is included as a generated label
(query results or exemplars). We don't have the option here because
metadata doesn't include any labels. Because of this we simply merge
and de-duplicate metadata for all tenants. The http.Handler will group
all metadata under each metric name.

Which issue(s) this PR fixes or relates to

Fixes #2410

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@56quarters 56quarters force-pushed the 56quarters/federated-metadata branch 2 times, most recently from 1217a0a to 72bc1f4 Compare July 19, 2022 17:03
Allow multiple tenant IDs to be passed to the `/api/v1/metadata`
endpoint in the same way we support for query and exemplar endpoints.

For other endpoints, the tenant ID is included as a generated label
(query results or exemplars). We don't have the option here because
metadata doesn't include any labels. Because of this we simply merge
metadata for all tenants. The `http.Handler` will group all metadata
under each metric name.

Fixes #2410

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
@56quarters 56quarters force-pushed the 56quarters/federated-metadata branch from 72bc1f4 to 44579de Compare July 19, 2022 23:03
@56quarters 56quarters marked this pull request as ready for review July 19, 2022 23:27
Copy link
Member

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

Good job!

I would suggest to remove testing multitenant configuration from tsdb-blocks-storage-s3, as it may be confusing for people who use it regularly.

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
@56quarters
Copy link
Contributor Author

Actually, I missed this part of the Prometheus docs that specifies that metadata has to be unique. I'll push a commit that fixes this shortly:

The data section of the query result consists of an object where each key is a metric name and each value is a list of unique metadata objects, as exposed for that metric name across all targets.

The following example returns two metrics. Note that the metric http_requests_total has more than one object in the list. At least one target has a value for HELP that do not match with the rest.

From https://prometheus.io/docs/prometheus/latest/querying/api/#querying-metric-metadata

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
Copy link
Contributor

@replay replay left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

I added one non-blocking comment

Copy link
Member

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

Good catch.

@56quarters 56quarters merged commit c54554c into main Jul 20, 2022
@56quarters 56quarters deleted the 56quarters/federated-metadata branch July 20, 2022 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Metadata endpoint does not support multiple tenants
3 participants