-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Added request logging for gRPC and HTTP in the server side #3361
Conversation
There seems to be a |
The test failures are unrelated to this PR. |
@yashrsharma44 Needs a rebase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks amazing generally just some nits.
Overall:
- Let's be consistent and create helpers for all components on one go.
- don't introduce non relevant changes (CHANGELOG reformat)
- Good work (:
I have used a |
Test Failures are unrelated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good 👍
Some thoughts during offline disscussions.
Feature Request: Would be nice if we add in the error message as well - https://cloud-native.slack.com/archives/CK5RSSC10/p1606754524365300 |
Currently, we have deployed a basic request logging for HTTP(#2849) and it seems to be working well. However, with the new addition of YAML config for request logging, it raises a question, how should we warn the user for the deprecating flag for request logging for HTTP.
Another issue is that, would we allow configuration from both the flags and YAML? Or one? |
Added the YAML configuration for request logging - Sample Config File - https://gist.github.com/yashrsharma44/02f5765c5710dd09ce5d14e854f22825
|
Currently, only gRPC provides logging of error, HTTP returns a status code for the same. If we want to log the error for HTTP, we need it from the response body, which is kind of blocking process and not worth it. I have added more fields for HTTP logs, which would help in finding the component which sends in the error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks super nice, thanks.
Generally lgtm, just minor nits.
Documentation of this new feature would be tracked in this issue #3623 😎 💪 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Some suggestions still.
High level suggestion:
- Please test things manually. I don't think flag filling works in current version at all
- Make sure we don't use panics or break consistency. Why suddenly we return error different request logging flag?
- Request logging is important but not the core of Thanos functionality. So why warning about deprecation of this flag on every Thanos run, even though only small portion is using this feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above
Can you elaborate on this -
|
Didn't we agree on panic quickly if configs not correct? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice.
Looks super close, but some things to improve still 🤗
Signed-off-by: Yash Sharma <[email protected]>
Signed-off-by: Yash Sharma <[email protected]>
Signed-off-by: Yash Sharma <[email protected]>
Signed-off-by: Yash Sharma <[email protected]>
Signed-off-by: Yash Sharma <[email protected]>
Signed-off-by: Yash Sharma <[email protected]>
Signed-off-by: Yash Sharma <[email protected]>
Signed-off-by: Yash Sharma <[email protected]>
Signed-off-by: Yash Sharma <[email protected]>
Signed-off-by: Yash Sharma <[email protected]>
Signed-off-by: Yash Sharma <[email protected]>
Signed-off-by: Yash Sharma <[email protected]>
… component starts Signed-off-by: Yash Sharma <[email protected]>
Signed-off-by: Yash Sharma <[email protected]>
…e as null Signed-off-by: Yash Sharma <[email protected]>
Signed-off-by: Yash Sharma <[email protected]>
Signed-off-by: Yash Sharma <[email protected]>
Signed-off-by: Yash Sharma <[email protected]>
Signed-off-by: Yash Sharma <[email protected]>
Signed-off-by: Yash Sharma <[email protected]>
Signed-off-by: Yash Sharma <[email protected]>
I had deleted the fork of Thanos for some reason, hence I am not able to push changes to this PR. |
Pull request was closed
cmd/thanos/query.go
Outdated
@@ -141,6 +144,17 @@ func registerQuery(app *extkingpin.App) { | |||
return errors.Errorf("Address %s is duplicated for --rule flag.", dup) | |||
} | |||
|
|||
// Check if the YAML configuration of request.logging is correct. Exit early if error. | |||
HTTPlogOpts, err := logging.DecideHTTPFlag(*reqLogDecision, reqLogConfig) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function should have verb
in name. Parse
Configure
would be better as you suggested offline.
…TP in the server side (#3862) * Added request logging for grpc Signed-off-by: Yash Sharma <[email protected]> Added request logging for grpc Signed-off-by: Yash Sharma <[email protected]> changed the logging option to log start and finish of a call Signed-off-by: Yash Sharma <[email protected]> added flags for enabling/disabling request logging for grpc Signed-off-by: Yash Sharma <[email protected]> nitpicks due to make docs Signed-off-by: Yash Sharma <[email protected]> added a changelog Signed-off-by: Yash Sharma <[email protected]> nitpicks Signed-off-by: Yash Sharma <[email protected]> configure option for logging Signed-off-by: Yash Sharma <[email protected]> changelog nitpick Signed-off-by: Yash Sharma <[email protected]> added changelog Signed-off-by: Yash Sharma <[email protected]> more nitpicks Signed-off-by: Yash Sharma <[email protected]> renamed requestLoggingDecision to reqLogDecision and some nitcpicks Signed-off-by: Yash Sharma <[email protected]> Added the check of reusing the request-id if present Signed-off-by: Yash Sharma <[email protected]> make docs nitpick Signed-off-by: Yash Sharma <[email protected]> Added the default level of debug and error codes for logging Signed-off-by: Yash Sharma <[email protected]> Add the levels for logging Signed-off-by: Yash Sharma <[email protected]> Added a YAML flag for request logging config Signed-off-by: Yash Sharma <[email protected]> Changed the setup signature to accept a reqlog param to be shared among all the components of Thanos. Changed the signature of decision for logging, and pushed the new yaml config among all the middleware Signed-off-by: Yash Sharma <[email protected]> Updated the grpc middleware package to include the latest signature of the decider Signed-off-by: Yash Sharma <[email protected]> Added new config files for YAML parsing Signed-off-by: Yash Sharma <[email protected]> changelog nitpick Signed-off-by: Yash Sharma <[email protected]> linting nitpicks Signed-off-by: Yash Sharma <[email protected]> Added deprecation warning and added a function to reroute logging through one of the flags Signed-off-by: Yash Sharma <[email protected]> docs nitpicks Signed-off-by: Yash Sharma <[email protected]> bug fix for reverse logging condition Signed-off-by: Yash Sharma <[email protected]> fixed some bugs for evaluating the options and added a warning for deprecating flag of log.request.decision Signed-off-by: Yash Sharma <[email protected]> removed gophercloud from go.mod Signed-off-by: Yash Sharma <[email protected]> self addressed comments Signed-off-by: Yash Sharma <[email protected]> * Added a dummy variable which was used by the, to be deprecated flag of request logging * Added a warning that request logging is disabled for tools * Removed a flag which is to be deprecated in the next release Signed-off-by: Yash Sharma <[email protected]> * Added a fail-first approach if the request logging is incorrectly enabled. Signed-off-by: Yash Sharma <[email protected]> Renaming of functions Signed-off-by: Yash Sharma <[email protected]> Removed indentation and simplified else conditions Signed-off-by: Yash Sharma <[email protected]> break down the yaml struct for grpc and http into its individual configs Signed-off-by: Yash Sharma <[email protected]> make docs nits Signed-off-by: Yash Sharma <[email protected]> modify changelog Signed-off-by: Yash Sharma <[email protected]> modified the signature of setup function to the original one Signed-off-by: Yash Sharma <[email protected]> removed the message for the flags Signed-off-by: Yash Sharma <[email protected]> make docs nits Signed-off-by: Yash Sharma <[email protected]> Added auto generation scripts for req logging Signed-off-by: Yash Sharma <[email protected]> removed request logging from compactor Signed-off-by: Yash Sharma <[email protected]> remove verbose warn messages Signed-off-by: Yash Sharma <[email protected]> changed pass by value to pass by reference Signed-off-by: Yash Sharma <[email protected]> removed occurence of os.Exit Signed-off-by: Yash Sharma <[email protected]> make docs nits Signed-off-by: Yash Sharma <[email protected]> revert compact.go to master Signed-off-by: Yash Sharma <[email protected]> rename ReqlogConfig to RequestConfig Signed-off-by: Yash Sharma <[email protected]> Added a validation check so that all the configs are checked before a component starts Signed-off-by: Yash Sharma <[email protected]> nits Signed-off-by: Yash Sharma <[email protected]> Modify the message for request.logging flag for having a default value as null Signed-off-by: Yash Sharma <[email protected]> remove a line from the flags Signed-off-by: Yash Sharma <[email protected]> remove a deceptive comment Signed-off-by: Yash Sharma <[email protected]> changed the var name to small caps Signed-off-by: Yash Sharma <[email protected]> change errors.Errorf to Wrapf for providing context to error message Signed-off-by: Yash Sharma <[email protected]> move changelog entry to unreleased tag Signed-off-by: Yash Sharma <[email protected]> changed request.logging to `request.logging` in the flags Signed-off-by: Yash Sharma <[email protected]> * added changelog entry Signed-off-by: Yash Sharma <[email protected]> * changed changelog Signed-off-by: Yash Sharma <[email protected]> * renamed decidehttpflag and decidegrpcflag Signed-off-by: Yash Sharma <[email protected]> * addressed reviewers comments Signed-off-by: Yash Sharma <[email protected]>
…PC and HTTP in the server side (thanos-io#3862) * Added request logging for grpc Signed-off-by: Yash Sharma <[email protected]> Added request logging for grpc Signed-off-by: Yash Sharma <[email protected]> changed the logging option to log start and finish of a call Signed-off-by: Yash Sharma <[email protected]> added flags for enabling/disabling request logging for grpc Signed-off-by: Yash Sharma <[email protected]> nitpicks due to make docs Signed-off-by: Yash Sharma <[email protected]> added a changelog Signed-off-by: Yash Sharma <[email protected]> nitpicks Signed-off-by: Yash Sharma <[email protected]> configure option for logging Signed-off-by: Yash Sharma <[email protected]> changelog nitpick Signed-off-by: Yash Sharma <[email protected]> added changelog Signed-off-by: Yash Sharma <[email protected]> more nitpicks Signed-off-by: Yash Sharma <[email protected]> renamed requestLoggingDecision to reqLogDecision and some nitcpicks Signed-off-by: Yash Sharma <[email protected]> Added the check of reusing the request-id if present Signed-off-by: Yash Sharma <[email protected]> make docs nitpick Signed-off-by: Yash Sharma <[email protected]> Added the default level of debug and error codes for logging Signed-off-by: Yash Sharma <[email protected]> Add the levels for logging Signed-off-by: Yash Sharma <[email protected]> Added a YAML flag for request logging config Signed-off-by: Yash Sharma <[email protected]> Changed the setup signature to accept a reqlog param to be shared among all the components of Thanos. Changed the signature of decision for logging, and pushed the new yaml config among all the middleware Signed-off-by: Yash Sharma <[email protected]> Updated the grpc middleware package to include the latest signature of the decider Signed-off-by: Yash Sharma <[email protected]> Added new config files for YAML parsing Signed-off-by: Yash Sharma <[email protected]> changelog nitpick Signed-off-by: Yash Sharma <[email protected]> linting nitpicks Signed-off-by: Yash Sharma <[email protected]> Added deprecation warning and added a function to reroute logging through one of the flags Signed-off-by: Yash Sharma <[email protected]> docs nitpicks Signed-off-by: Yash Sharma <[email protected]> bug fix for reverse logging condition Signed-off-by: Yash Sharma <[email protected]> fixed some bugs for evaluating the options and added a warning for deprecating flag of log.request.decision Signed-off-by: Yash Sharma <[email protected]> removed gophercloud from go.mod Signed-off-by: Yash Sharma <[email protected]> self addressed comments Signed-off-by: Yash Sharma <[email protected]> * Added a dummy variable which was used by the, to be deprecated flag of request logging * Added a warning that request logging is disabled for tools * Removed a flag which is to be deprecated in the next release Signed-off-by: Yash Sharma <[email protected]> * Added a fail-first approach if the request logging is incorrectly enabled. Signed-off-by: Yash Sharma <[email protected]> Renaming of functions Signed-off-by: Yash Sharma <[email protected]> Removed indentation and simplified else conditions Signed-off-by: Yash Sharma <[email protected]> break down the yaml struct for grpc and http into its individual configs Signed-off-by: Yash Sharma <[email protected]> make docs nits Signed-off-by: Yash Sharma <[email protected]> modify changelog Signed-off-by: Yash Sharma <[email protected]> modified the signature of setup function to the original one Signed-off-by: Yash Sharma <[email protected]> removed the message for the flags Signed-off-by: Yash Sharma <[email protected]> make docs nits Signed-off-by: Yash Sharma <[email protected]> Added auto generation scripts for req logging Signed-off-by: Yash Sharma <[email protected]> removed request logging from compactor Signed-off-by: Yash Sharma <[email protected]> remove verbose warn messages Signed-off-by: Yash Sharma <[email protected]> changed pass by value to pass by reference Signed-off-by: Yash Sharma <[email protected]> removed occurence of os.Exit Signed-off-by: Yash Sharma <[email protected]> make docs nits Signed-off-by: Yash Sharma <[email protected]> revert compact.go to master Signed-off-by: Yash Sharma <[email protected]> rename ReqlogConfig to RequestConfig Signed-off-by: Yash Sharma <[email protected]> Added a validation check so that all the configs are checked before a component starts Signed-off-by: Yash Sharma <[email protected]> nits Signed-off-by: Yash Sharma <[email protected]> Modify the message for request.logging flag for having a default value as null Signed-off-by: Yash Sharma <[email protected]> remove a line from the flags Signed-off-by: Yash Sharma <[email protected]> remove a deceptive comment Signed-off-by: Yash Sharma <[email protected]> changed the var name to small caps Signed-off-by: Yash Sharma <[email protected]> change errors.Errorf to Wrapf for providing context to error message Signed-off-by: Yash Sharma <[email protected]> move changelog entry to unreleased tag Signed-off-by: Yash Sharma <[email protected]> changed request.logging to `request.logging` in the flags Signed-off-by: Yash Sharma <[email protected]> * added changelog entry Signed-off-by: Yash Sharma <[email protected]> * changed changelog Signed-off-by: Yash Sharma <[email protected]> * renamed decidehttpflag and decidegrpcflag Signed-off-by: Yash Sharma <[email protected]> * addressed reviewers comments Signed-off-by: Yash Sharma <[email protected]>
Fixes #2844
Signed-off-by: Yash Sharma [email protected]
Changes
v2
. We are using the last commit which contains the logging middleware. Oncegrpc_middleware
releases thev2
we can add that as well.Verification
Current Output -
Current TODO
cc @kakkoyun 🤓