-
Notifications
You must be signed in to change notification settings - Fork 505
Add a new filter that logs inbound requests to Namerd #2275
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
Conversation
Signed-off-by: Dennis Adjei-Baah <dennis@buoyant.io>
Signed-off-by: Dennis Adjei-Baah <dennis@buoyant.io>
919f275
to
81b8935
Compare
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. WDYT about moving the configuration logic up out of the finagle/h2 library and into the mesh initializer?
import java.net.InetSocketAddress | ||
|
||
object AccessLogger { | ||
def mk( |
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.
This method which processes option properties and applies defaults feels out of place in a pure library like this. I think this logic would fit better directly in MeshIfaceInitializer.
val filter = logFilePath match { | ||
case Some(path) if path != "" => | ||
val logger = LoggerFactory( | ||
node = "access_io.l5d.mesh", |
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.
I don't think this logger node name is appropriate for an arbitrary access logger, but if you move this into the mesh iface initializer, I think it makes much more sense.
Signed-off-by: Dennis Adjei-Baah <dennis@buoyant.io>
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 good, pending two small nits
var h2AccessLogRotateCount: Option[Int] = None | ||
|
||
@JsonIgnore | ||
def mkAccessLogger( |
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.
can this be private?
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.
sure thing!
val append = logAppend.getOrElse(true) | ||
val logRotate = logRotateCount.getOrElse(-1) | ||
|
||
val filter = logFilePath match { |
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.
why save this to a val and then immediately return it?
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.
I think it may be left over from me debugging this piece of code. Good catch.
Signed-off-by: Dennis Adjei-Baah <dennis@buoyant.io>
In order to see what requests are being made to a Namerd process, this PR adds a
SimpleFilter
to the mesh interface to log every inbound request to Namerd in the Common Log Format.Running a docker compose environment with Linkerd and Namerd, resolving service names in Linkerd's dtab playground yields these log lines in Namerd.
Fixes #2255
Signed-off-by: Dennis Adjei-Baah dennis@buoyant.io