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

Bring back RED metrics for querier when processing scheduler requests. #11097

Merged
merged 2 commits into from
Nov 1, 2023

Conversation

jeschkies
Copy link
Contributor

What this PR does / why we need it:
A previous change removed the RED metrics for the querier. This adds them back as part of a middleware.

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 a team as a code owner October 31, 2023 16:53
@@ -702,6 +702,25 @@ func (c Codec) EncodeRequest(ctx context.Context, r queryrangebase.Request) (*ht
}
}

func (c Codec) Path(r queryrangebase.Request) string {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this could be used by EncodePath as well.

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.

LGTM, tested in dev as well. We get method=GET for /metrics, /ready, and /tail, method=grpc for the rest. Route label looks fine /loki/api/v1/... becomes loki_api_v1_... as expected.

@jeschkies jeschkies added type/bug Somehing is not working as expected backport k173 labels Nov 1, 2023
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.

Looks reasonable 👍
Have we compared the metrics produced with the previous ones?
Showing a clean diff would be tablestakes for merging this I think.

@jeschkies
Copy link
Contributor Author

Callum verified that the metrics are present. I can create a diff

@jeschkies
Copy link
Contributor Author

I've Loki locally with k172 and this patch.

curl -v -H "X-Scope-OrgID: 1" -G http://localhost:8004/metrics | cut -f 1 -d " " | sort | uniq > main.log
curl -v -H "X-Scope-OrgID: 1" -G http://localhost:8004/metrics | cut -f 1 -d " " | sort | uniq > k172.log
diff k172.log main.log > diff.log

Here's the diff.log. We are missing loki_request_message_bytes_bucket and loki_response_message_bytes_bucket. However, this patch is not addressing these.

@dannykopping
Copy link
Contributor

dannykopping commented Nov 1, 2023

I've Loki locally with k172 and this patch.

curl -v -H "X-Scope-OrgID: 1" -G http://localhost:8004/metrics | cut -f 1 -d " " | sort | uniq > main.log
curl -v -H "X-Scope-OrgID: 1" -G http://localhost:8004/metrics | cut -f 1 -d " " | sort | uniq > k172.log
diff k172.log main.log > diff.log

Here's the diff.log. We are missing loki_request_message_bytes_bucket and loki_response_message_bytes_bucket. However, this patch is not addressing these.

Can you attach the raw logs as well pls?

@jeschkies
Copy link
Contributor Author

Here is the raw output from the /metrics endpoint, @dannykopping
metrics_k172.log
metrics_patch.log

@dannykopping
Copy link
Contributor

I dropped the sample values from all the metrics to make the comparison purely on the series themselves.

$  awk 'NF{NF-=1};1' <metrics_k172.log >before.log
$  awk 'NF{NF-=1};1' <metrics_patch.log >after.log
$ diff -u {before,after}.log > diff.txt

I've attached the diff here. Most of it looks right but I'm between a few threads this morning and you have most context.
I'll approve the PR and if the diff looks good to you then merge it.

diff.txt

@jeschkies jeschkies merged commit 33b7e51 into grafana:main Nov 1, 2023
3 checks passed
@jeschkies jeschkies deleted the karsten/querier-red-metrics branch November 1, 2023 10:52
jeschkies added a commit that referenced this pull request Nov 1, 2023
#11097)

**What this PR does / why we need it**:
A previous change removed the RED metrics for the querier. This adds
them back as part of a middleware.

**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)
jeschkies added a commit that referenced this pull request Nov 1, 2023
#11097)

**What this PR does / why we need it**:
A previous change removed the RED metrics for the querier. This adds
them back as part of a middleware.

**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
grafana#11097)

**What this PR does / why we need it**:
A previous change removed the RED metrics for the querier. This adds
them back as part of a middleware.

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