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

inflight-logging: Add extra metadata to inflight requests logging #11243

Merged
merged 7 commits into from
Nov 20, 2023

Conversation

kavirajk
Copy link
Contributor

@kavirajk kavirajk commented Nov 16, 2023

What this PR does / why we need it:
logging: Add extra metadata to inflight requests

This adds extra metadata (similar to what we have in metrics.go) but for queries in in-flight (both started and retrying)

Changes:
Adds following data
1. Query Hash
2. Start and End time
3. Start and End delta
4. Length of the query
5. Moved the helper util to queryutil package because of cyclic dependencies with logql package.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
Find the screenshots of log entries looks like (both in retry.go and roundtrip.go)

Screenshot 2023-11-16 at 13 01 32
Screenshot 2023-11-16 at 13 02 15

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

This adds extra metadata (similar to what we have in `metrics.go`) but for queries in in-flight (both started and retrying)

Changes:
Adds following data
1. Query Hash
2. Start and End time
3. Start and End delta
4. Length of the query

Signed-off-by: Kaviraj <[email protected]>
@kavirajk kavirajk requested a review from a team as a code owner November 16, 2023 12:05
Copy link
Contributor

Trivy scan found the following vulnerabilities:

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, left a nit about package structure though

@@ -0,0 +1,10 @@
package queryutil
Copy link
Contributor

Choose a reason for hiding this comment

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

This package doesn't match the naming convention we have in pkg/util. Maybe just move it to the util package, since we have pkg/util/hash_fp.go already.

Signed-off-by: Kaviraj <[email protected]>
@pull-request-size pull-request-size bot added size/L and removed size/M labels Nov 20, 2023
Signed-off-by: Kaviraj <[email protected]>
Signed-off-by: Kaviraj <[email protected]>
@kavirajk kavirajk enabled auto-merge (squash) November 20, 2023 09:25
Signed-off-by: Kaviraj <[email protected]>
@kavirajk kavirajk force-pushed the kavirajk/improve-inflight-logging branch from 73c3a58 to b7d7d4a Compare November 20, 2023 09:46
@kavirajk kavirajk merged commit 30d0030 into main Nov 20, 2023
7 checks passed
@kavirajk kavirajk deleted the kavirajk/improve-inflight-logging branch November 20, 2023 10:00
MichelHollands pushed a commit that referenced this pull request Nov 21, 2023
…1243)

**What this PR does / why we need it**:
logging: Add extra metadata to inflight requests

This adds extra metadata (similar to what we have in `metrics.go`) but
for queries in in-flight (both started and retrying)

Changes:
    Adds following data
    1. Query Hash
    2. Start and End time
    3. Start and End delta
    4. Length of the query
5. Moved the helper util to `queryutil` package because of cyclic
dependencies with `logql` package.
   
**Which issue(s) this PR fixes**:
Fixes #<issue number>

**Special notes for your reviewer**:
Find the screenshots of log entries looks like (both in `retry.go` and
`roundtrip.go`)

![Screenshot 2023-11-16 at 13 01
32](https://github.com/grafana/loki/assets/3735252/177e97ed-6ee8-41dd-b088-2e4f49562ba0)
![Screenshot 2023-11-16 at 13 02
15](https://github.com/grafana/loki/assets/3735252/fb328a37-dbe3-483e-b083-f21327858029)

**Checklist**
- [x] 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](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: Kaviraj <[email protected]>
jeschkies pushed a commit that referenced this pull request Nov 21, 2023
…1243)

**What this PR does / why we need it**:
logging: Add extra metadata to inflight requests

This adds extra metadata (similar to what we have in `metrics.go`) but
for queries in in-flight (both started and retrying)

Changes:
    Adds following data
    1. Query Hash
    2. Start and End time
    3. Start and End delta
    4. Length of the query
5. Moved the helper util to `queryutil` package because of cyclic
dependencies with `logql` package.
   
**Which issue(s) this PR fixes**:
Fixes #<issue number>

**Special notes for your reviewer**:
Find the screenshots of log entries looks like (both in `retry.go` and
`roundtrip.go`)

![Screenshot 2023-11-16 at 13 01
32](https://github.com/grafana/loki/assets/3735252/177e97ed-6ee8-41dd-b088-2e4f49562ba0)
![Screenshot 2023-11-16 at 13 02
15](https://github.com/grafana/loki/assets/3735252/fb328a37-dbe3-483e-b083-f21327858029)

**Checklist**
- [x] 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](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: Kaviraj <[email protected]>
rhnasc pushed a commit to inloco/loki that referenced this pull request Apr 12, 2024
…afana#11243)

**What this PR does / why we need it**:
logging: Add extra metadata to inflight requests

This adds extra metadata (similar to what we have in `metrics.go`) but
for queries in in-flight (both started and retrying)

Changes:
    Adds following data
    1. Query Hash
    2. Start and End time
    3. Start and End delta
    4. Length of the query
5. Moved the helper util to `queryutil` package because of cyclic
dependencies with `logql` package.
   
**Which issue(s) this PR fixes**:
Fixes #<issue number>

**Special notes for your reviewer**:
Find the screenshots of log entries looks like (both in `retry.go` and
`roundtrip.go`)

![Screenshot 2023-11-16 at 13 01
32](https://github.com/grafana/loki/assets/3735252/177e97ed-6ee8-41dd-b088-2e4f49562ba0)
![Screenshot 2023-11-16 at 13 02
15](https://github.com/grafana/loki/assets/3735252/fb328a37-dbe3-483e-b083-f21327858029)

**Checklist**
- [x] 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)
- [ ] 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: Kaviraj <[email protected]>
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.

2 participants