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

Log filtering framework client #608

Merged
merged 11 commits into from
Jul 17, 2019
Merged

Conversation

tejaswiagarwal
Copy link
Contributor

There was no mechanism to filter logs in the client. This adds a new framework in client using ContextExtractor to filter logs.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 67.126% when pulling 4728e88 on log_filtering_framework_client into 403dedf on master.

@coveralls
Copy link

coveralls commented Jul 10, 2019

Coverage Status

Coverage increased (+0.5%) to 67.62% when pulling b35c8b8 on log_filtering_framework_client into 403dedf on master.

runtime/tchannel_client.go Outdated Show resolved Hide resolved
runtime/tchannel_client.go Outdated Show resolved Hide resolved
runtime/tchannel_interfaces.go Outdated Show resolved Hide resolved
runtime/tchannel_client.go Outdated Show resolved Hide resolved
@@ -158,8 +158,6 @@ func TestCallTChannelSuccessfulRequestOKResponse(t *testing.T) {
"hostname",
"pid",
"timestamp-finished",
"Client-Req-Header-x-request-uuid",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these fields deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code to add "Client-Req-Header" was removed from here: https://github.com/uber/zanzibar/pull/608/files/4728e886495b2d2fa9f1d0a92a3d83b0c92947a9#diff-63b43474c6820d3db553746e13b0e82fL92

Hence, these logs will not be generated anymore.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any filter added to remove those from the logs, what happened here is that endpoint and client have the same header keys, and putting them on the context has an overwrite effect. We need the prefix to differentiate between those.

@ChuntaoLu ChuntaoLu requested review from jacobgreenleaf and removed request for herainman July 12, 2019 21:02
runtime/tchannel_outbound_call.go Outdated Show resolved Hide resolved
var logFields []zap.Field
if c.client != nil && c.client.contextExtractor != nil {
headers := map[string]string{}
for k, v := range c.reqHeaders {
Copy link
Contributor

Choose a reason for hiding this comment

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

This only includes request headers, what about response headers?
Also we need the prefix to differentiate the header keys between client and endpoint

@@ -158,8 +158,6 @@ func TestCallTChannelSuccessfulRequestOKResponse(t *testing.T) {
"hostname",
"pid",
"timestamp-finished",
"Client-Req-Header-x-request-uuid",
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any filter added to remove those from the logs, what happened here is that endpoint and client have the same header keys, and putting them on the context has an overwrite effect. We need the prefix to differentiate between those.

Copy link
Contributor

@ChuntaoLu ChuntaoLu left a comment

Choose a reason for hiding this comment

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

Almost there, just some test change that we should not do.

@@ -62,14 +62,9 @@ func getRequestTags(ctx context.Context) map[string]string {
func getRequestFields(ctx context.Context) []zap.Field {
var fields []zap.Field
headers := zanzibar.GetEndpointRequestHeadersFromCtx(ctx)
uuid, ok := headers["X-Uuid"]
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not change this section, we deliberately avoided to have token as log fields here.

"Deviceversion": "1.0",
"Device": "ios",
"Regionname": "sf",
"x-token": "token",
Copy link
Contributor

Choose a reason for hiding this comment

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

Token is still logged?

@tejaswiagarwal tejaswiagarwal merged commit 8eac3ef into master Jul 17, 2019
@ChuntaoLu ChuntaoLu deleted the log_filtering_framework_client branch July 8, 2020 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants