Document request logging configuration #4934

philipgough · 2021-12-08T17:02:44Z

Fixes #4861

I added CHANGELOG entry for this change.
Change is not relevant to the end user.

Changes

Verification

bwplotka · 2021-12-08T17:30:18Z

You might need to run make docs to auto-gen docs (:

yeya24 · 2021-12-11T21:54:57Z

docs/logging.md

+http:
+  config:
+    - path: /api/v1/query
+      port: 10904


What's this port config for?

That would be coming from https://github.com/thanos-io/thanos/blob/main/pkg/logging/yaml_parser.go#L34

However I did not look too deeply into the implementation. Just double checked the original example https://gist.github.com/yashrsharma44/02f5765c5710dd09ce5d14e854f22825 against what was exposed by those types.

It would be nice to document this field in more detail. Maybe @yashrsharma44 can help answer this. I feel confused when I saw this config TBH.

Agree, I just don't have enough context to provide more info. I had a look at #3862 but it didn't tell me much.

I believe this is to specify which port to log the requests on - this might make sense in the context of components exposing multiple HTTP endpoints (e.g. receiver). But I'm also thinking if we could make the user experience here more intuitive and somehow figure out which HTTP server it is based on paths? 🤔

@dwilliams782 it is looking for an exact match. regex matching etc is not supported - I've documented the behaviour on https://github.com/thanos-io/thanos/pull/4934/files#diff-1a5e57d66b08bec5a71860d67f06912a004f03368c36910099d4ffae12970c7dR61

Omitting the config will match all as you have seen.

@philipgough how do we know what an exact match is? AFAIK my config above should be an "exact" match, in that, they are the three endpoints being emitted (when running without config) and that is my HTTP port. How else can I diagnose this?

@dwilliams782 - the exact match would include any query params etc - I think it would be beneficial to be able to add \/api\/v1\/query\?.+ for example, but that is for another issues.

And I am sorry I don't have all the answers since I put this together by stumbling through the same pain points as you are right now :)

My diagnosis was done by running locally and outputting logs and stepping through the debugger - not aware of a better way.

Ah I see! Perhaps your example of using config should specify that any parameters need to match too? That wasn't obvious to me.

Yeah, I really appreciate your efforts here. I'm not expecting you to know the answers, but hopefully asking the questions can help improve the documentation when we do stumble on the answers. Cheers!

Yeah no worries, good point, I can add a couple of examples with query params to the example conf and it might make it more obvious

By the way @matej-g has created #4961 to track some of the issues and figure out improvements to this so might be worth keeping an eye on that going forward or logging any suggestions you have (not for existing config/implementation) for improving things.

matej-g

So glad we'll have this important feature documented better ❤️ Adding few comments.

docs/logging.md

matej-g · 2021-12-14T16:51:02Z

docs/logging.md

+http:
+  config:
+    - path: /api/v1/query
+      port: 10904


I believe this is to specify which port to log the requests on - this might make sense in the context of components exposing multiple HTTP endpoints (e.g. receiver). But I'm also thinking if we could make the user experience here more intuitive and somehow figure out which HTTP server it is based on paths? 🤔

matej-g

This is great, thanks for digging in and documenting the behavior! Just few smallish suggestions.

matej-g · 2021-12-21T17:11:02Z

docs/logging.md

+
+## Configuration
+
+Configuration can be supplied for either gRPC, HTTP or both. Options can be supplied globally, which is applied to both gRPC and HTTP, or individually to either or both protocols.


Suggested change

Configuration can be supplied for either gRPC, HTTP or both. Options can be supplied globally, which is applied to both gRPC and HTTP, or individually to either or both protocols.

Configuration can be supplied for either gRPC, HTTP or both. Options can be supplied globally, which is applied to both gRPC and HTTP, or individually to either of both protocols.

I think my wording is correct here actually but I see it can be confusing. Does this work for you:

Configuration can be supplied globally which applies to both `grpc` and `http`. Alternatively `http` and/or `grpc` can be configured independently.

Sounds good!

docs/logging.md

matej-g · 2021-12-21T17:14:22Z

docs/logging.md

+
+### Options
+
+Valid `level` for `options` should be one of `INFO`, `WARNING`, `ERROR` or `DEBUG`.


So what does level set? 🤔

So level, from what I can see provides an additional filter. Take the following example

I have enabled --log.level=debug globally and request logging is enabled with ERROR - I will not see any of the debug logs emitted from the http middleware.

This could be seen as a nuisance or as a feature :)

Understood, then let's keep it!

matej-g

I think this is looking good now, great improvement! Good job @philipgough!

dwilliams782 · 2022-02-01T11:03:29Z

I've found this PR when trying to work out how the logging config works. Thanks a lot for your effort!

philipgough · 2022-02-01T11:47:23Z

docs/logging.md

+* Log details prior to the request and when the request completes - Pre request log is made at `debug` level.
+
+```yaml
+options:


IIUC - the default is ERROR - https://github.com/thanos-io/thanos/blob/main/pkg/logging/grpc.go#L45

wiardvanrij · 2022-02-05T13:48:01Z

Hi thanks. Let's get this merged. Could you rebase from main as we've changed some tests and these are now missing?

philipgough · 2022-02-08T10:23:51Z

@wiardvanrij - rebased - docs check seems to be failing but not sure that it is related if you wouldn't mind running the check again?

wiardvanrij · 2022-02-08T12:44:30Z

@wiardvanrij - rebased - docs check seems to be failing but not sure that it is related if you wouldn't mind running the check again?

Re-ran the job but it really has a diff in it. It should make a change on your doc when you run it locally, that needs to be committed. I can take a look later, because I lack a bit of time at the moment to help you out right now.

Signed-off-by: Philip Gough <philip.p.gough@gmail.com>

philipgough · 2022-02-08T14:19:55Z

Re-ran the job but it really has a diff in it. It should make a change on your doc when you run it locally, that needs to be committed.

Noted, I ran make docs and it did indeed produce a diff but it feels like it is incorrect regardless..I've committed the results so lets see what the check shows

wiardvanrij · 2022-02-08T14:34:52Z

@philipgough , are you sure you have rebased correctly? We made some changes in our docs/formatting. Perhaps manually diff your Makefile with that of ours in main?
Ah, --- ignore this, what @matej-g said :x

Erm, @matej-g why we have the docs? Maybe we can make it better so we don't have this confusion? (lets follow up? :D )

matej-g · 2022-02-08T14:34:59Z

Noted, I ran make docs and it did indeed produce a diff but it feels like it is incorrect regardless..I've committed the results so lets see what the check shows

I think you need to run make check-docs, as this is what the CI check is using, as it does a couple of extra steps, including removing white noise.

philipgough · 2022-02-08T14:41:08Z

~~@matej-g - will try that, although the output of the github action is run make docs and commit changes~~

solution seems to have been brew install gsed

Signed-off-by: Philip Gough <philip.p.gough@gmail.com>

matej-g

Everything is passing now 🎉 May we requests a merge @wiardvanrij? 🙂

wiardvanrij · 2022-02-09T17:54:42Z

Yes, this is better than what we have now. Thanks so much @philipgough and sorry for the bad experience on getting all the tests done :(

P.s. If people find things that are not correct or require extra input, I encourage those to iterate on this document and create a PR to improve it. For now, let's move forward.

philipgough · 2022-02-09T17:58:15Z

P.s. If people find things that are not correct or require extra input, I encourage those to iterate on this document and create a PR to improve it. For now, let's move forward.

My thoughts exactly. There were some suggestions for changes but I think getting this over the line firs tis a good step. Thanks @wiardvanrij, @matej-g

* docs: Add instructions for configuring request logging Signed-off-by: Philip Gough <philip.p.gough@gmail.com> * docs: Update link to logging config and make docs Signed-off-by: Philip Gough <philip.p.gough@gmail.com> Signed-off-by: Nicholaswang <wzhever@gmail.com>

philipgough force-pushed the query-log branch 2 times, most recently from 6a3bde2 to 04338a5 Compare December 8, 2021 17:22

philipgough force-pushed the query-log branch from 04338a5 to 793cf9d Compare December 9, 2021 14:30

pull-request-size bot added the size/L label Dec 9, 2021

yeya24 reviewed Dec 11, 2021

View reviewed changes

matej-g reviewed Dec 14, 2021

View reviewed changes

philipgough mentioned this pull request Dec 15, 2021

logging: Allow request logging config to determine log level #4956

Closed

2 tasks

matej-g mentioned this pull request Dec 16, 2021

Logging: Simplify and adjust request logging configuration #4961

Open

philipgough marked this pull request as draft December 20, 2021 17:24

philipgough force-pushed the query-log branch 2 times, most recently from f4277f0 to 743e872 Compare December 21, 2021 11:02

philipgough marked this pull request as ready for review December 21, 2021 11:03

philipgough mentioned this pull request Dec 21, 2021

manifests: Support thanos query requst logging via CM openshift/cluster-monitoring-operator#1513

Merged

2 tasks

matej-g reviewed Dec 21, 2021

View reviewed changes

philipgough force-pushed the query-log branch from 7c0b856 to 71ae8d9 Compare December 22, 2021 10:35

matej-g previously approved these changes Jan 21, 2022

View reviewed changes

philipgough dismissed matej-g’s stale review via dfd88d4 January 26, 2022 17:20

philipgough force-pushed the query-log branch from 71ae8d9 to dfd88d4 Compare January 26, 2022 17:20

philipgough commented Feb 1, 2022

View reviewed changes

philipgough force-pushed the query-log branch 2 times, most recently from 5d45cde to 133ae8b Compare February 7, 2022 16:20

pull-request-size bot added size/XL and removed size/L labels Feb 7, 2022

philipgough force-pushed the query-log branch from 133ae8b to dfe0249 Compare February 8, 2022 10:10

pull-request-size bot added size/L and removed size/XL labels Feb 8, 2022

pull-request-size bot added size/XL and removed size/L labels Feb 8, 2022

docs: Add instructions for configuring request logging

57e8eda

Signed-off-by: Philip Gough <philip.p.gough@gmail.com>

philipgough force-pushed the query-log branch from 26ab0d0 to 53724fd Compare February 8, 2022 14:18

docs: Update link to logging config and make docs

5406919

Signed-off-by: Philip Gough <philip.p.gough@gmail.com>

philipgough force-pushed the query-log branch from 53724fd to 5406919 Compare February 8, 2022 15:22

pull-request-size bot added size/L and removed size/XL labels Feb 8, 2022

matej-g approved these changes Feb 9, 2022

View reviewed changes

wiardvanrij enabled auto-merge (squash) February 9, 2022 17:54

wiardvanrij approved these changes Feb 9, 2022

View reviewed changes

wiardvanrij merged commit c038534 into thanos-io:main Feb 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Document request logging configuration #4934

Document request logging configuration #4934

philipgough commented Dec 8, 2021

bwplotka commented Dec 8, 2021 •

edited

yeya24 Dec 11, 2021

philipgough Dec 12, 2021

yeya24 Dec 12, 2021

philipgough Dec 14, 2021

matej-g Dec 14, 2021

philipgough Feb 1, 2022

dwilliams782 Feb 2, 2022

philipgough Feb 2, 2022

dwilliams782 Feb 2, 2022

philipgough Feb 2, 2022 •

edited

matej-g left a comment

matej-g Dec 14, 2021

matej-g left a comment

matej-g Dec 21, 2021

philipgough Dec 21, 2021 •

edited

matej-g Jan 21, 2022

matej-g Dec 21, 2021

philipgough Dec 21, 2021

matej-g Jan 21, 2022

matej-g left a comment

dwilliams782 commented Feb 1, 2022

philipgough Feb 1, 2022

wiardvanrij commented Feb 5, 2022

philipgough commented Feb 8, 2022

wiardvanrij commented Feb 8, 2022

philipgough commented Feb 8, 2022

wiardvanrij commented Feb 8, 2022 •

edited

matej-g commented Feb 8, 2022 •

edited

philipgough commented Feb 8, 2022 •

edited

matej-g left a comment

wiardvanrij commented Feb 9, 2022

philipgough commented Feb 9, 2022


		## Configuration

		Configuration can be supplied for either gRPC, HTTP or both. Options can be supplied globally, which is applied to both gRPC and HTTP, or individually to either or both protocols.


		### Options

		Valid `level` for `options` should be one of `INFO`, `WARNING`, `ERROR` or `DEBUG`.

Document request logging configuration #4934

Document request logging configuration #4934

Conversation

philipgough commented Dec 8, 2021

Changes

Verification

bwplotka commented Dec 8, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philipgough Feb 2, 2022 • edited

Choose a reason for hiding this comment

matej-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matej-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philipgough Dec 21, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matej-g left a comment

Choose a reason for hiding this comment

dwilliams782 commented Feb 1, 2022

Choose a reason for hiding this comment

wiardvanrij commented Feb 5, 2022

philipgough commented Feb 8, 2022

wiardvanrij commented Feb 8, 2022

philipgough commented Feb 8, 2022

wiardvanrij commented Feb 8, 2022 • edited

matej-g commented Feb 8, 2022 • edited

philipgough commented Feb 8, 2022 • edited

matej-g left a comment

Choose a reason for hiding this comment

wiardvanrij commented Feb 9, 2022

philipgough commented Feb 9, 2022

bwplotka commented Dec 8, 2021 •

edited

philipgough Feb 2, 2022 •

edited

philipgough Dec 21, 2021 •

edited

wiardvanrij commented Feb 8, 2022 •

edited

matej-g commented Feb 8, 2022 •

edited

philipgough commented Feb 8, 2022 •

edited