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

Fix single-tenant metrics not to filter by tenant #7385

Merged
merged 2 commits into from
May 24, 2024
Merged

Conversation

fantix
Copy link
Member

@fantix fantix commented May 23, 2024

This is a bug introduced in #7024 that /metrics returns different results using HTTP or HTTPS protocol under single-tenant mode. This PR fixes so that /metrics never filter by tenant under single-tenant mode.

This PR also fixes the test to fetch_metrics() with TLS certificate and adds a test to make sure edgedb_server_compiler_processes_current - as an example - is not filtered out.

@msullivan
Copy link
Member

Though there is also the question of: should we be removing the tenant label when filtering by tenant at all?

@zackelan
Copy link
Contributor

should we be removing the tenant label when filtering by tenant at all?

I would prefer not to - if querying metrics for tenant X does not include the tenant label, then at the point where we scrape the metrics, we would have to go out of our way to add the tenant label back in, so that in our metrics storage system per-tenant metrics are queryable. since the server already knows the per-tenant metrics, there's very little point in stripping the label out only to immediately add it back.

@fantix
Copy link
Member Author

fantix commented May 23, 2024

Sounds good, I'll add the label back.

@fantix fantix merged commit 248e1f6 into master May 24, 2024
23 checks passed
@fantix fantix deleted the single-tenant-metrics branch May 24, 2024 14:37
@msullivan msullivan added backported-5.x PRs that *have* been backported to 5.x (starting with 5.3) and removed to-backport-5.x labels Jul 1, 2024
msullivan pushed a commit that referenced this pull request Jul 1, 2024
* Fix single-tenant metrics not to filter by tenant
* Always print all labels in /metrics
msullivan pushed a commit that referenced this pull request Jul 1, 2024
* Fix single-tenant metrics not to filter by tenant
* Always print all labels in /metrics
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported-5.x PRs that *have* been backported to 5.x (starting with 5.3)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants