Skip to content

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

Merged
merged 4 commits into from
May 29, 2019

Conversation

dadjeibaah
Copy link
Contributor

@dadjeibaah dadjeibaah commented May 15, 2019

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.

namerd_1   | D 0515 15:24:51.108 UTC THREAD19:  172.18.0.2 - - [15/05/2019:15:24:51 +0000] "POST /io.linkerd.mesh.Delegator/StreamDtab HTTP/2" 200 - "-" "-"
namerd_1   | D 0515 15:24:51.953 UTC THREAD19:  172.18.0.2 - - [15/05/2019:15:24:51 +0000] "POST /io.linkerd.mesh.Delegator/StreamDtab HTTP/2" 200 - "-" "-"
namerd_1   | D 0515 15:25:09.219 UTC THREAD19:  172.18.0.2 - - [15/05/2019:15:25:09 +0000] "POST /io.linkerd.mesh.Delegator/GetDelegateTree HTTP/2" 200 - "-" "-"
namerd_1   | D 0515 15:25:09.262 UTC THREAD19:  172.18.0.2 - - [15/05/2019:15:25:09 +0000] "POST /io.linkerd.mesh.Resolver/StreamReplicas HTTP/2" 200 - "-" "-"

Fixes #2255

Signed-off-by: Dennis Adjei-Baah dennis@buoyant.io

Signed-off-by: Dennis Adjei-Baah <dennis@buoyant.io>
@dadjeibaah dadjeibaah requested a review from adleong May 15, 2019 15:27
@dadjeibaah dadjeibaah requested a review from olix0r as a code owner May 15, 2019 15:27
@dadjeibaah dadjeibaah self-assigned this May 15, 2019
Signed-off-by: Dennis Adjei-Baah <dennis@buoyant.io>
@dadjeibaah dadjeibaah force-pushed the dad/namerd-access-log branch from 919f275 to 81b8935 Compare May 23, 2019 22:11
Copy link
Member

@adleong adleong left a 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(
Copy link
Member

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",
Copy link
Member

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>
Copy link
Member

@adleong adleong left a 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(
Copy link
Member

Choose a reason for hiding this comment

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

can this be private?

Copy link
Contributor Author

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 {
Copy link
Member

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?

Copy link
Contributor Author

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>
@dadjeibaah dadjeibaah merged commit 5dd234e into master May 29, 2019
@dadjeibaah dadjeibaah deleted the dad/namerd-access-log branch May 29, 2019 18:37
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.

Namerd access log
2 participants