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.

Sign up for GitHub

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

Jump to bottom

WIP: separate interface into immutable and mutable #5

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

WIP: separate interface into immutable and mutable #5

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion jvm/core/src/main/kotlin/com/m3/tracing/M3Tracer.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package com.m3.tracing

import com.m3.tracing.http.HttpRequestInfo
import com.m3.tracing.http.HttpRequestSpan
import com.m3.tracing.http.MutableHttpRequestInfo
import javax.annotation.CheckReturnValue
import javax.annotation.concurrent.ThreadSafe

Expand Down Expand Up @@ -34,7 +35,7 @@ interface M3Tracer: AutoCloseable, TraceContext {
* Caller MUST close the [HttpRequestSpan] to prevent leak.
*/
@CheckReturnValue
fun processOutgoingHttpRequest(request: HttpRequestInfo): HttpRequestSpan
fun processOutgoingHttpRequest(request: MutableHttpRequestInfo): HttpRequestSpan

/**
* Returns context bound to current thread / call stack.
Expand Down
5 changes: 0 additions & 5 deletions jvm/core/src/main/kotlin/com/m3/tracing/http/HttpRequestInfo.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,6 @@ interface HttpRequestInfo {
* @return If given header is not available, return null.
*/
fun tryGetHeader(name: String): String?

/**
* Set value into header.
*/
fun trySetHeader(name: String, value: String)
}

/**
Expand Down
16 changes: 16 additions & 0 deletions jvm/core/src/main/kotlin/com/m3/tracing/http/MutableHttpRequestInfo.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package com.m3.tracing.http

/**
* Represents mutable HTTP request.
* Implementation depends on framework (Servlet, Play Framework, ...).
*
* This interface is intended NOT to consume stream of request body to prevent breaking application.
* Thus this interface does NOT provide any information depends on request body (includes request "parameter").
*/
interface MutableHttpRequestInfo : HttpRequestInfo {
/**
* Set value into header.
* Should restrict name. Especially encoding/charset header must NOT be modified.
*/
fun trySetHeader(name: String, value: String)
}
3 changes: 2 additions & 1 deletion jvm/core/src/main/kotlin/com/m3/tracing/tracer/logging/M3LoggingTracer.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import com.m3.tracing.TraceSpan
import com.m3.tracing.http.HttpRequestInfo
import com.m3.tracing.http.HttpRequestSpan
import com.m3.tracing.http.HttpResponseInfo
import com.m3.tracing.http.MutableHttpRequestInfo
import org.slf4j.LoggerFactory

/**
Expand Down Expand Up @@ -38,7 +39,7 @@ class M3LoggingTracer: M3Tracer {
override fun startChildSpan(name: String): TraceSpan = TraceSpanImpl(name)
}

override fun processOutgoingHttpRequest(request: HttpRequestInfo): HttpRequestSpan = object: TraceSpanImpl("HTTP ${request.url}"), HttpRequestSpan {
override fun processOutgoingHttpRequest(request: MutableHttpRequestInfo): HttpRequestSpan = object: TraceSpanImpl("HTTP ${request.url}"), HttpRequestSpan {
private var e: Throwable? = null
override fun setError(e: Throwable?) {
this.e = e
Expand Down
10 changes: 5 additions & 5 deletions jvm/opencensus/src/main/kotlin/com/m3/tracing/tracer/opencensus/HttpRequestTracer.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ internal class HttpRequestTracer(
}
}
@VisibleForTesting
internal val setter = object: TextFormat.Setter<HttpRequestInfo>() {
override fun put(carrier: HttpRequestInfo, key: String, value: String) {
internal val setter = object: TextFormat.Setter<MutableHttpRequestInfo>() {
override fun put(carrier: MutableHttpRequestInfo, key: String, value: String) {
return carrier.trySetHeader(key, value)
}
}
Expand All @@ -38,7 +38,7 @@ internal class HttpRequestTracer(
it.init()
}

fun processClientRequest(request: HttpRequestInfo) = HttpClientRequestSpanImpl(clientHandler, tracer, request).also {
fun processClientRequest(request: MutableHttpRequestInfo) = HttpClientRequestSpanImpl(clientHandler, tracer, request).also {
it.init()
}
}
Expand Down Expand Up @@ -100,9 +100,9 @@ internal class HttpRequestSpanImpl(
}

internal class HttpClientRequestSpanImpl(
private val handler: HttpClientHandler<HttpRequestInfo, HttpResponseInfo, HttpRequestInfo>,
private val handler: HttpClientHandler<HttpRequestInfo, HttpResponseInfo, MutableHttpRequestInfo>,
override val tracer: Tracer,
private val request: HttpRequestInfo
private val request: MutableHttpRequestInfo
): TraceSpanImpl(null), HttpRequestSpan {
companion object {
private val logger = LoggerFactory.getLogger(HttpRequestTracer::class.java)
Expand Down
3 changes: 2 additions & 1 deletion jvm/opencensus/src/main/kotlin/com/m3/tracing/tracer/opencensus/M3OpenCensusTracer.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import com.m3.tracing.TraceContext
import com.m3.tracing.TraceSpan
import com.m3.tracing.http.HttpRequestInfo
import com.m3.tracing.http.HttpRequestSpan
import com.m3.tracing.http.MutableHttpRequestInfo
import com.m3.tracing.internal.Config
import io.grpc.Context
import io.opencensus.common.Scope
Expand Down Expand Up @@ -65,7 +66,7 @@ class M3OpenCensusTracer internal constructor(

override fun processIncomingHttpRequest(request: HttpRequestInfo): HttpRequestSpan = httpRequestTracer.processRequest(request)

override fun processOutgoingHttpRequest(request: HttpRequestInfo): HttpRequestSpan = httpRequestTracer.processClientRequest(request)
override fun processOutgoingHttpRequest(request: MutableHttpRequestInfo): HttpRequestSpan = httpRequestTracer.processClientRequest(request)
Copy link
Contributor

@saiya saiya Oct 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Advanced recommendation: How about renaming existing HttpRequestInfo to IncomingHttpRequestInfo and also rename MutableHttpRequestInfo to OutgoingHttpRequestInfo ?

And make common super-interface HttpRequestInfo to avoid copy-and-paste (IncomingHttpRequestInfo extends HttpRequestInfo and OutgoingHttpRequestInfo extends HttpRequestInfo).

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, that's nice! 👍


private fun createSampler() = SamplerFactory.createSampler(
Config[samplingConfigName] ?: "never"
Expand Down
1 change: 0 additions & 1 deletion jvm/servlet/src/main/kotlin/com/m3/tracing/tracer/servlet/ServletHttpRequestInfo.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ open class ServletHttpRequestInfo(protected val req: HttpServletRequest): HttpRe
// Thus this filter should not do anything breaks setCharacterEncoding() even after FilterCain.

override fun tryGetHeader(name: String): String? = req.getHeader(name)
override fun trySetHeader(name: String, value: String) = Unit // Do nothing

@Suppress("UNCHECKED_CAST", "IMPLICIT_ANY")
override fun <T> tryGetMetadata(key: HttpRequestMetadataKey<T>): T? = when(key) {
Expand Down
19 changes: 17 additions & 2 deletions jvm/spring-web/src/main/kotlin/com/m3/tracing/spring/http/client/SpringHttpRequestInfo.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,26 @@ package com.m3.tracing.spring.http.client

import com.m3.tracing.http.HttpRequestInfo
import com.m3.tracing.http.HttpRequestMetadataKey
import com.m3.tracing.http.MutableHttpRequestInfo
import org.slf4j.LoggerFactory
import org.springframework.http.HttpRequest

open class SpringHttpRequestInfo(protected val req: HttpRequest): HttpRequestInfo {
open class SpringHttpRequestInfo(protected val req: HttpRequest): MutableHttpRequestInfo {
companion object {
private val acceptableHeaders = listOf(
"traceparent", "tracestate",
"X-B3-TraceId", "X-B3-SpanId", "X-B3-Sampled",
"X-Cloud-Trace-Context"
)
private val logger = LoggerFactory.getLogger(SpringHttpRequestInfo::class.java)
}

override fun tryGetHeader(name: String): String? = req.headers.getFirst(name)
override fun trySetHeader(name: String, value: String) = req.headers.set(name, value)
override fun trySetHeader(name: String, value: String) = if (acceptableHeaders.contains(name)) {
req.headers.set(name, value)
} else {
logger.error("Failed to set header name: ${name}, value: ${value}, it's not accepatable")
}

@Suppress("UNCHECKED_CAST", "IMPLICIT_ANY")
override fun <T> tryGetMetadata(key: HttpRequestMetadataKey<T>): T? = when(key) {
Expand Down