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: make detected fields work for both json and proto #12682

Merged
merged 13 commits into from
Apr 19, 2024

Conversation

trevorwhitney
Copy link
Collaborator

@trevorwhitney trevorwhitney commented Apr 18, 2024

What this PR does / why we need it:

This makes the new /detected_fields endpoint work for both JSON and protobuf encodings.

#12491 adding splitting of detected fields queries. This is accomplished by serializing the hyperloglog sketches built in the queriers in the response so they can be merged in the query frontends. We don't want the serialized sketch in the JSON response from the frontend, so #12491 used a bit of a hack of defining a - json tag. This had the tradeoff of only allowing detected field splitting to work when protobuf encoding was enabled. Because of this, we guarded the registration of the route on whether or not protobuf was set as the encoding type.

This decision has already tripped some people up since protobuf in not the default, and raised concerns about OSS users, so this PR changes the strategy for removing the serialized sketches to one that works with both protofbuf and json encodings.

I also added an integration test, which identified a bug in streamsForFieldDetection that this PR also fixes.

@trevorwhitney trevorwhitney requested a review from a team as a code owner April 18, 2024 18:31
@trevorwhitney trevorwhitney marked this pull request as draft April 19, 2024 01:44
@pull-request-size pull-request-size bot added size/L and removed size/M labels Apr 19, 2024
@trevorwhitney trevorwhitney marked this pull request as ready for review April 19, 2024 01:53
@@ -1610,7 +1610,7 @@ func (Codec) MergeResponse(responses ...queryrangebase.Response) (queryrangebase
return &DetectedFieldsResponse{
Response: &logproto.DetectedFieldsResponse{
Fields: mergedFields,
FieldLimit: fieldLimit,
FieldLimit: 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

is this intentional, hardcoding to 0?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, the FieldLimit is only needed in the merge code, so once we've merged we don't need to pass that back to the user

Comment on lines 453 to 456
message DetectedFieldsResponse {
repeated DetectedField fields = 1;
uint32 fieldLimit = 2;
uint32 fieldLimit = 2 [(gogoproto.jsontag) = "fieldLimit,omitempty"];
}
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we be defining an alternative message type for inter-service usage only, and using the base DetectedFieldsResponse as the external response type?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ideally yes, but I don't know how to do that without having to implement a new method on both the querier and query frontend, which also feels messy since the query frontend shouldn't be concerned with returning internal representations of anything.

I think I could probably learn from the quantile/probabilistic query types code you did. I can circle back on this once I've had a chance to get more familiar with that?

Copy link
Contributor

@cstyan cstyan left a comment

Choose a reason for hiding this comment

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

discussed on slack, LGTM

@trevorwhitney trevorwhitney enabled auto-merge (squash) April 19, 2024 22:22
@trevorwhitney trevorwhitney merged commit f68d1f7 into main Apr 19, 2024
14 checks passed
@trevorwhitney trevorwhitney deleted the detected-fields-fixes branch April 19, 2024 22:28
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.

2 participants