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
Define QueryResponse and QueryRequest protobufs. #10956
Define QueryResponse and QueryRequest protobufs. #10956
Conversation
func HttpgrpcToHTTPResponse(resp *httpgrpc.HTTPResponse) *http.Response {
httpResp := &http.Response{
StatusCode: int(resp.Code),
Body: &buffer{buff: resp.Body, ReadCloser: io.NopCloser(bytes.NewReader(resp.Body))},
Header: http.Header{},
ContentLength: int64(len(resp.Body)),
}
for _, h := range resp.Headers {
httpResp.Header[h.Key] = h.Values
}
return httpResp
}
func HTTPtoHttpgrpcResponse(resp *http.Response) (*httpgrpc.HTTPResponse, error) {
var buf []byte
var err error
if buffer, ok := resp.Body.(Buffer); ok {
buf = buffer.Bytes()
} else {
buf, err = io.ReadAll(resp.Body)
if err != nil {
return nil, httpgrpc.Errorf(http.StatusInternalServerError, "error decoding response: %v", err)
}
}
grpcResp := &httpgrpc.HTTPResponse{
Code: int32(resp.StatusCode),
Body: buf,
}
for k, v := range resp.Header {
grpcResp.Headers = append(grpcResp.Headers, &httpgrpc.Header{Key: k, Values: v})
}
return grpcResp, nil
} |
Are the todos in the opening of the PR still relevant? |
…y-protos' into karsten/enable-query-protos
@cstyan they are all done. |
791ee3b
to
54976d2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Minor non-blocking comments
Let's land it 🚀
# The downstream querier is required to answer in the accepted format. Can be | ||
# 'json' or 'protobuf'. Note: Both will still be routed over GRPC. | ||
# CLI flag: -frontend.required-query-response-format | ||
[required_query_response_format: <string> | default = "json"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this implemented? I don't see it
It can also be a note in the upgrade guide if it's a pain
httpReq, err := f.codec.EncodeRequest(ctx, req) | ||
if err != nil { | ||
return nil, fmt.Errorf("cannot convert request to HTTP request: %w", err) | ||
} | ||
|
||
if err := user.InjectOrgIDIntoHTTPRequest(ctx, httpReq); err != nil { | ||
return nil, httpgrpc.Errorf(http.StatusBadRequest, err.Error()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking:
Was surprised by this; shouldn't we automatically inject the orgID into the request when encoding it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome catch!
**What this PR does / why we need it**: This is a follow up to #10956. `lokigrpc.GetParentSpanForRequest` would ignore `opentracing.ErrSpanContextNotFound`. However, that should be up to the caller to decide whether this error should be ignored or not. **Checklist** - [ ] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [ ] Documentation added - [ ] Tests updated - [ ] `CHANGELOG.md` updated - [ ] If the change is worth mentioning in the release notes, add `add-to-release-notes` label - [ ] Changes that require user attention or interaction to upgrade are documented in `docs/sources/setup/upgrade/_index.md` - [ ] For Helm chart changes bump the Helm chart version in `production/helm/loki/Chart.yaml` and update `production/helm/loki/CHANGELOG.md` and `production/helm/loki/README.md`. [Example PR](d10549e) - [ ] If the change is deprecating or removing a configuration option, update the `deprecated-config.yaml` and `deleted-config.yaml` files respectively in the `tools/deprecated-config-checker` directory. [Example PR](0d4416a)
**What this PR does / why we need it**: This will be a follow up to grafana#10688 finally switching over to protos. Hang on tight! Everything is behind the feature flag `-frontend.encoding=protobuf` which will disable the transcoding to HTTP and HTTPgRPC. **Checklist** - [ ] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [ ] Documentation added - [x] Tests updated - [x] `CHANGELOG.md` updated - [ ] If the change is worth mentioning in the release notes, add `add-to-release-notes` label - [ ] Changes that require user attention or interaction to upgrade are documented in `docs/sources/setup/upgrade/_index.md` - [ ] For Helm chart changes bump the Helm chart version in `production/helm/loki/Chart.yaml` and update `production/helm/loki/CHANGELOG.md` and `production/helm/loki/README.md`. [Example PR](grafana@d10549e) --------- Signed-off-by: Callum Styan <callumstyan@gmail.com> Co-authored-by: Callum Styan <callumstyan@gmail.com> Co-authored-by: Danny Kopping <dannykopping@gmail.com>
**What this PR does / why we need it**: This is a follow up to grafana#10956. `lokigrpc.GetParentSpanForRequest` would ignore `opentracing.ErrSpanContextNotFound`. However, that should be up to the caller to decide whether this error should be ignored or not. **Checklist** - [ ] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [ ] Documentation added - [ ] Tests updated - [ ] `CHANGELOG.md` updated - [ ] If the change is worth mentioning in the release notes, add `add-to-release-notes` label - [ ] Changes that require user attention or interaction to upgrade are documented in `docs/sources/setup/upgrade/_index.md` - [ ] For Helm chart changes bump the Helm chart version in `production/helm/loki/Chart.yaml` and update `production/helm/loki/CHANGELOG.md` and `production/helm/loki/README.md`. [Example PR](grafana@d10549e) - [ ] If the change is deprecating or removing a configuration option, update the `deprecated-config.yaml` and `deleted-config.yaml` files respectively in the `tools/deprecated-config-checker` directory. [Example PR](grafana@0d4416a)
What this PR does / why we need it:
This will be a follow up to #10688 finally switching over to protos. Hang on tight!
TODO
Checklist
CONTRIBUTING.md
guide (required)CHANGELOG.md
updatedadd-to-release-notes
labeldocs/sources/setup/upgrade/_index.md
production/helm/loki/Chart.yaml
and updateproduction/helm/loki/CHANGELOG.md
andproduction/helm/loki/README.md
. Example PR