Skip to content

Commit

Permalink
finagle-http2: Disable FrameLoggers by default
Browse files Browse the repository at this point in the history
Problem

`FrameLoggers` are running by default for clients despite not being
used the overwhelming majority of the time.

Solution

Disable them by default. Service owners can enable them with the
`c.t.f.http2.param.FrameLogging.Enabled` Stack Param. For example:
`Http.client.configured(FrameLogging.Enabled)`.

JIRA Issues: CSL-7976

Differential Revision: https://phabricator.twitter.biz/D326727
  • Loading branch information
kevinoliver authored and jenkins committed Jun 13, 2019
1 parent 6a1e13a commit 0b2ec20
Show file tree
Hide file tree
Showing 4 changed files with 48 additions and 6 deletions.
10 changes: 7 additions & 3 deletions CHANGELOG.rst
Expand Up @@ -35,6 +35,9 @@ New Features
Runtime Behavior Changes
~~~~~~~~~~~~~~~~~~~~~~~~

* finagle: Upgrade to Netty 4.1.35.Final and netty-tcnative 2.0.25.Final.
``PHAB_ID=D312439``

* finagle-core: The default failure accrual policy has been changed from one
which uses only consecutive failures to a hybrid model which uses both
success rate over a window and consecutive failures. Previously this was
Expand All @@ -46,12 +49,13 @@ Runtime Behavior Changes
populated when `isChunked` is set to true in the request and response respectively.
``PHAB_ID=D315041``

* finagle: Upgrade to Netty 4.1.35.Final and netty-tcnative 2.0.25.Final.
``PHAB_ID=D312439``

* finagle-http2: Disable ping-based failure detector in HTTP/2 client as it seems to do
more harm than good. ``PHAB_ID=D322357``

* finagle-http2: Frame logging is now disabled by default for clients. To enable,
use the `c.t.f.http2.param.FrameLogging.Enabled` Stack Param. For example:
`Http.client.configured(FrameLogging.Enabled)`. ``PHAB_ID=D326727``

* finagle-netty4: When using a Netty `LocalChannel`, the value of the `BackPressure`
stack param is effectively changed to `backPressureDisabled` so that other functionality
(e.g. SSL/TLS) works as expected. ``PHAB_ID=D319011``
Expand Down
Expand Up @@ -59,7 +59,6 @@ private[finagle] object Http2Transporter {

val connectionHandlerBuilder = new RichHttpToHttp2ConnectionHandlerBuilder()
.frameListener(new Http2ClientDowngrader(connection))
.frameLogger(new LoggerPerFrameTypeLogger(params[FrameLoggerNamePrefix].loggerNamePrefix))
.connection(connection)
.initialSettings(Settings.fromParams(params, isServer = false))
.encoderIgnoreMaxHeaderListSize(ignoreMaxHeaderListSize)
Expand All @@ -69,6 +68,11 @@ private[finagle] object Http2Transporter {
}
})

if (params[FrameLogging].enabled) {
connectionHandlerBuilder.frameLogger(
new LoggerPerFrameTypeLogger(params[FrameLoggerNamePrefix].loggerNamePrefix))
}

val PriorKnowledge(priorKnowledge) = params[PriorKnowledge]
val Transport.ClientSsl(config) = params[Transport.ClientSsl]
val tlsEnabled = config.isDefined
Expand Down
Expand Up @@ -4,6 +4,7 @@ import com.twitter.finagle.Stack
import com.twitter.finagle.http2.param.{
EncoderIgnoreMaxHeaderListSize,
FrameLoggerNamePrefix,
FrameLogging,
HeaderSensitivity
}
import com.twitter.finagle.param.Stats
Expand Down Expand Up @@ -95,19 +96,23 @@ private object MultiplexCodecBuilder {
inboundInitializer: ChannelHandler,
isServer: Boolean
): Http2MultiplexCodecBuilder = {
val logger = new LoggerPerFrameTypeLogger(params[FrameLoggerNamePrefix].loggerNamePrefix)
val initialSettings = Settings.fromParams(params, isServer = isServer)
val builder: Http2MultiplexCodecBuilder =
if (isServer) Http2MultiplexCodecBuilder.forServer(inboundInitializer)
else Http2MultiplexCodecBuilder.forClient(inboundInitializer)

builder
.frameLogger(logger)
.initialSettings(initialSettings)
.encoderIgnoreMaxHeaderListSize(
params[EncoderIgnoreMaxHeaderListSize].ignoreMaxHeaderListSize
)
.headerSensitivityDetector(detector(params))

if (params[FrameLogging].enabled) {
builder.frameLogger(
new LoggerPerFrameTypeLogger(params[FrameLoggerNamePrefix].loggerNamePrefix))
}
builder
}

// Build a sensitivity detector from the params
Expand Down
Expand Up @@ -140,6 +140,35 @@ object HeaderSensitivity {
implicit val param = Stack.Param(HeaderSensitivity(NeverSensitive))
}

/**
* Whether or not HTTP/2 frame logging is enabled.
*
* Defaults to disabled.
*
* @see `Enabled` and `Disabled` on companion class for getting instances.
*/
final case class FrameLogging private (enabled: Boolean) {
def mk(): (FrameLogging, Stack.Param[FrameLogging]) =
(this, FrameLogging.param)
}

object FrameLogging {

/**
* Frame logging is disabled.
*/
val Disabled: FrameLogging = FrameLogging(false)

/**
* Frame logging is enabled.
*
* @see [[FrameLoggerNamePrefix]] for further configuration.
*/
val Enabled: FrameLogging = FrameLogging(true)

implicit val param: Stack.Param[FrameLogging] = Stack.Param(Disabled)
}

/**
* The logger name to be used for the root HTTP/2 frame logger. This allows each frame type
* to be turned on and off by changing the level of prefix.<FRAME_TYPE>, or turning everything
Expand Down

0 comments on commit 0b2ec20

Please sign in to comment.