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

Instrument querier inflight requests. #11085

Merged

Conversation

jeschkies
Copy link
Contributor

@jeschkies jeschkies commented Oct 30, 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 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
  • 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

@jeschkies jeschkies requested a review from slim-bean October 30, 2023 16:33
@jeschkies jeschkies requested a review from a team as a code owner October 30, 2023 16:33
@jeschkies jeschkies force-pushed the karsten/instrument-querier-inflight-requests branch from 21216cc to 2fdace3 Compare October 31, 2023 07:21
@pull-request-size pull-request-size bot added size/M and removed size/S labels Oct 31, 2023
func NewQueryHandlerMetrics(registerer prometheus.Registerer, metricsNamespace string) *QueryHandlerMetrics {
return &QueryHandlerMetrics{
InflightRequests: promauto.With(registerer).NewGaugeVec(prometheus.GaugeOpts{
Namespace: metricsNamespace,
Copy link
Contributor

Choose a reason for hiding this comment

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

should we set this to loki?

metricsNamespace was added for migrating metrics that were using cortex prefix. But in this case it was already set to loki I believe, so there is no need to make the prefix configurable now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it should stay flexible but thanks for the notice.

Copy link
Contributor

@ashwanthgoli ashwanthgoli left a comment

Choose a reason for hiding this comment

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

lgtm

@jeschkies jeschkies merged commit 8742599 into grafana:main Oct 31, 2023
5 checks passed
@jeschkies jeschkies deleted the karsten/instrument-querier-inflight-requests branch October 31, 2023 10:42
@grafanabot
Copy link
Collaborator

Hello @jeschkies!
Backport pull requests need to be either:

  • Pull requests which address bugs,
  • Urgent fixes which need product approval, in order to get merged,
  • Docs changes.

Please, if the current pull request addresses a bug fix, label it with the type/bug label.
If it already has the product approval, please add the product-approved label. For docs changes, please add the type/docs label.
If the pull request modifies CI behaviour, please add the type/ci label.
If none of the above applies, please consider removing the backport label and target the next major/minor release.
Thanks!

@jeschkies jeschkies added type/bug Somehing is not working as expected and removed missing-labels labels Oct 31, 2023
jeschkies added a commit to jeschkies/loki that referenced this pull request Oct 31, 2023
**What this PR does / why we need it**:
means inflight requests are not instrumented anymore. This change adds a
middleware that does that.

This will add metrics such as
```
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)
jeschkies added a commit that referenced this pull request Oct 31, 2023
**What this PR does / why we need it**:
* Instrument querier inflight requests. (#11085)
* Recover querier handler from panic. (#10983)

**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)
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport k173 size/M type/bug Somehing is not working as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants