fix: make detected fields work for both json and proto #12682
Conversation
| @@ -1610,7 +1610,7 @@ func (Codec) MergeResponse(responses ...queryrangebase.Response) (queryrangebase | |||
| return &DetectedFieldsResponse{ | |||
| Response: &logproto.DetectedFieldsResponse{ | |||
| Fields: mergedFields, | |||
| FieldLimit: fieldLimit, | |||
| FieldLimit: 0, | |||
There was a problem hiding this comment.
is this intentional, hardcoding to 0?
There was a problem hiding this comment.
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
| message DetectedFieldsResponse { | ||
| repeated DetectedField fields = 1; | ||
| uint32 fieldLimit = 2; | ||
| uint32 fieldLimit = 2 [(gogoproto.jsontag) = "fieldLimit,omitempty"]; | ||
| } |
There was a problem hiding this comment.
shouldn't we be defining an alternative message type for inter-service usage only, and using the base DetectedFieldsResponse as the external response type?
There was a problem hiding this comment.
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?
trevorwhitney
force-pushed
the
detected-fields-fixes
branch
from
127bc72
to
efafd51
Compare
What this PR does / why we need it:
This makes the new
/detected_fieldsendpoint 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 whenprotobufencoding was enabled. Because of this, we guarded the registration of the route on whether or notprotobufwas set as the encoding type.This decision has already tripped some people up since
protobufin 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 bothprotofbufandjsonencodings.I also added an integration test, which identified a bug in
streamsForFieldDetectionthat this PR also fixes.