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

Support protobuf QueryRequest in querier. #10858

Merged
merged 50 commits into from
Oct 18, 2023

Conversation

jeschkies
Copy link
Contributor

What this PR does / why we need it:
This is one step to support pure protobuf encoding without httpgrpc. It is only on the scheduler and is fully backwards compatible.

Which issue(s) this PR fixes:
This is a sub change of #10688

Checklist

  • Reviewed the 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

pkg/querier/handler.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@trevorwhitney trevorwhitney left a comment

Choose a reason for hiding this comment

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

Not much to add here that I didn't address in #10688. Approving to unblock, but it looks like there's still a bit of cleanup to do before merging.

Thanks!

@jeschkies
Copy link
Contributor Author

The PR passed the correctness test https://ops.grafana-ops.net/a/k6-app/runs/2014651

Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

LGTM

samples: make([]record.RefSample, 0, 100),
exemplars: make([]record.RefExemplar, 0, 10),
w: storage,
notify: notify,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can get rid of these changes now?

@jeschkies jeschkies merged commit 9fcc42d into grafana:main Oct 18, 2023
4 checks passed
@jeschkies jeschkies deleted the karsten/protos-query-request branch October 18, 2023 14:16
jeschkies added a commit that referenced this pull request Oct 31, 2023
**What this PR does / why we need it**:
#10858 removed the HTTP and gRPC server from the querier workers. That
means inflight requests are not instrumented anymore. This change adds a
middleware that does that.

This will add metrics such as 
```
# HELP cortex_inflight_requests Current number of inflight requests.
# TYPE cortex_inflight_requests gauge
cortex_inflight_requests{method="gRPC",route="*logproto.IndexStatsRequest"} 0
cortex_inflight_requests{method="gRPC",route="*queryrange.LabelRequest"} 0
cortex_inflight_requests{method="gRPC",route="*queryrange.LokiRequest"} 0
cortex_inflight_requests{method="gRPC",route="*queryrange.LokiSeriesRequest"} 0
```

**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)
dannykopping pushed a commit that referenced this pull request Nov 7, 2023
**What this PR does / why we need it**:
#10858 removed the code that would
inject query tags into the context of the querier. This change adds an
extraction in the decode method.
grafanabot pushed a commit that referenced this pull request Nov 7, 2023
**What this PR does / why we need it**:
#10858 removed the code that would
inject query tags into the context of the querier. This change adds an
extraction in the decode method.

(cherry picked from commit 6d95a86)
slim-bean pushed a commit that referenced this pull request Nov 7, 2023
Backport 6d95a86 from #11147

---

**What this PR does / why we need it**:
#10858 removed the code that would
inject query tags into the context of the querier. This change adds an
extraction in the decode method.

**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)

Co-authored-by: Karsten Jeschkies <karsten.jeschkies@grafana.com>
slim-bean added a commit that referenced this pull request Nov 8, 2023
**What this PR does / why we need it**:
#10858 removed the extraction of the
query time header on the querier side and the generation of the cache
number. This change adds them back and uses the headers of the response
format.

**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)

---------

Signed-off-by: Edward Welch <edward.welch@grafana.com>
Co-authored-by: Edward Welch <edward.welch@grafana.com>
grafanabot pushed a commit that referenced this pull request Nov 8, 2023
**What this PR does / why we need it**:
#10858 removed the extraction of the
query time header on the querier side and the generation of the cache
number. This change adds them back and uses the headers of the response
format.

**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)

---------

Signed-off-by: Edward Welch <edward.welch@grafana.com>
Co-authored-by: Edward Welch <edward.welch@grafana.com>
(cherry picked from commit 979530b)
slim-bean pushed a commit that referenced this pull request Nov 8, 2023
Backport 979530b from #11176

---

**What this PR does / why we need it**:
#10858 removed the extraction of the
query time header on the querier side and the generation of the cache
number. This change adds them back and uses the headers of the response
format.

**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)

Co-authored-by: Karsten Jeschkies <karsten.jeschkies@grafana.com>
rhnasc pushed a commit to inloco/loki that referenced this pull request Apr 12, 2024
**What this PR does / why we need it**:
This is one step to support pure protobuf encoding without `httpgrpc`.
It is only on the scheduler and is fully backwards compatible.

**Which issue(s) this PR fixes**:
This is a sub change of grafana#10688

**Checklist**
- [ ] Reviewed the
[`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md)
guide (**required**)
- [ ] Documentation added
- [x] 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)

---------

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
Co-authored-by: Kaviraj <kavirajkanagaraj@gmail.com>
Co-authored-by: Danny Kopping <dannykopping@gmail.com>
rhnasc pushed a commit to inloco/loki that referenced this pull request Apr 12, 2024
**What this PR does / why we need it**:
grafana#10858 removed the HTTP and gRPC server from the querier workers. That
means inflight requests are not instrumented anymore. This change adds a
middleware that does that.

This will add metrics such as 
```
# HELP cortex_inflight_requests Current number of inflight requests.
# TYPE cortex_inflight_requests gauge
cortex_inflight_requests{method="gRPC",route="*logproto.IndexStatsRequest"} 0
cortex_inflight_requests{method="gRPC",route="*queryrange.LabelRequest"} 0
cortex_inflight_requests{method="gRPC",route="*queryrange.LokiRequest"} 0
cortex_inflight_requests{method="gRPC",route="*queryrange.LokiSeriesRequest"} 0
```

**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)
rhnasc pushed a commit to inloco/loki that referenced this pull request Apr 12, 2024
**What this PR does / why we need it**:
grafana#10858 removed the code that would
inject query tags into the context of the querier. This change adds an
extraction in the decode method.
rhnasc pushed a commit to inloco/loki that referenced this pull request Apr 12, 2024
**What this PR does / why we need it**:
grafana#10858 removed the extraction of the
query time header on the querier side and the generation of the cache
number. This change adds them back and uses the headers of the response
format.

**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)

---------

Signed-off-by: Edward Welch <edward.welch@grafana.com>
Co-authored-by: Edward Welch <edward.welch@grafana.com>
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.

None yet

5 participants