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 method/path/protocol for http request #70

Merged
merged 2 commits into from Feb 8, 2019

Conversation

Projects
None yet
4 participants
@song-jiang
Copy link
Member

song-jiang commented Feb 7, 2019

Description

Removed unnecessary logging. Log method/path/protocol/source/destination for http request.

Todos

  • Tests
  • Documentation
  • Release note

Release Note

Removed unnecessary logging in Dikastes for HTTP requests.

@song-jiang song-jiang requested a review from projectcalico/core-maintainers as a code owner Feb 7, 2019

@song-jiang song-jiang requested a review from spikecurtis Feb 7, 2019

@spikecurtis
Copy link
Member

spikecurtis left a comment

The only HTTP headers that Dikastes uses are the method and path, but there are other things in the request that we want to log like the source and destination (address, port, principal).

"Req.Method": req.Request.GetAttributes().GetRequest().GetHttp().GetMethod(),
"Req.Path": req.Request.GetAttributes().GetRequest().GetHttp().GetPath(),
"Req.Protocol": req.Request.GetAttributes().GetRequest().GetHttp().GetProtocol(),
}).Info("Checking rule on request")

This comment has been minimized.

@spikecurtis

spikecurtis Feb 7, 2019

Member

Let's keep these at Debug level

"Req.Method": req.GetAttributes().GetRequest().GetHttp().GetMethod(),
"Req.Path": req.GetAttributes().GetRequest().GetHttp().GetPath(),
"Req.Protocol": req.GetAttributes().GetRequest().GetHttp().GetProtocol(),
}).Info("Check start")

This comment has been minimized.

@spikecurtis

spikecurtis Feb 7, 2019

Member

Keep at debug level

"Req.Method": req.GetAttributes().GetRequest().GetHttp().GetMethod(),
"Req.Path": req.GetAttributes().GetRequest().GetHttp().GetPath(),
"Req.Protocol": req.GetAttributes().GetRequest().GetHttp().GetProtocol(),
"Response": resp,

This comment has been minimized.

@spikecurtis

spikecurtis Feb 7, 2019

Member

not your bug, but I think this "check complete" log should also be at Debug level. At Info level it means we are writing logs for every request, even if debug logging is turned off, which might hurt performance.

@song-jiang song-jiang merged commit 0eff6c4 into projectcalico:master Feb 8, 2019

2 checks passed

license/cla Contributor License Agreement is signed.
Details
semaphoreci The build passed on Semaphore.
Details

@song-jiang song-jiang deleted the song-jiang:song-fix-log branch Feb 8, 2019

@caseydavenport caseydavenport added this to the Calico v3.6.0 milestone Feb 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.