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 (v2): add support for ProfileTypes requests #3541

Merged
merged 4 commits into from
Sep 5, 2024

Conversation

aleks-p
Copy link
Contributor

@aleks-p aleks-p commented Sep 4, 2024

ProfileTypes is still used for datasource health checks (see this), therefore we need to have it supported in the v2 read path as well. Adding a simple implementation that collects profile types from the block metadata datasets.

TODO: add unit test

@aleks-p aleks-p requested a review from a team as a code owner September 4, 2024 20:35
@@ -138,9 +138,9 @@ func (q *QueryBackend) read(
}

func (q *QueryBackend) withThrottling(fn func() (*queryv1.InvokeResponse, error)) (*queryv1.InvokeResponse, error) {
defer q.running.Dec()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is unrelated, but a previous query-backend based implementation hit an issue where the number of running queries kept going up once the limit was hit

}
return connect.NewResponse(&querierv1.ProfileTypesResponse{}), nil
return Query[querierv1.ProfileTypesRequest, querierv1.ProfileTypesResponse](ctx, r, c,
func(a, b *querierv1.ProfileTypesResponse) (*querierv1.ProfileTypesResponse, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this merges data coming from the v1 and the v2 read paths, when the time range includes the transition between the two

Copy link
Collaborator

@korniltsev korniltsev left a comment

Choose a reason for hiding this comment

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

I wonder if we should ask frontend to switch to some other method for healthcheck. I susupect this is the only usage of the method. I had the same issue during POC

@aleks-p
Copy link
Contributor Author

aleks-p commented Sep 5, 2024

I wonder if we should ask frontend to switch to some other method for healthcheck. I susupect this is the only usage of the method. I had the same issue during POC

I agree and I can make the switch to a different method in Grafana. Since it is a public API though and would be used in older Grafana versions we can only mark it as deprecated for now but not remove it.

@aleks-p aleks-p force-pushed the fix/v2/add-profile-types-request branch from 6c93b83 to eb1d328 Compare September 5, 2024 18:36
@aleks-p aleks-p merged commit 26210f5 into main Sep 5, 2024
18 checks passed
@aleks-p aleks-p deleted the fix/v2/add-profile-types-request branch September 5, 2024 19:24
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.

2 participants