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

Add library instrumentation for ktor server #4983

Merged
merged 1 commit into from
Jan 5, 2022

Conversation

anuraaga
Copy link
Contributor

For #4972

This seemed like a fun instrumentation to try to get more practice with Kotlin. It wasn't fun when trying to integrate it with Spock :P

import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming
import kotlinx.coroutines.withContext

class KtorServerTracing private constructor(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@anuraaga anuraaga changed the title Add library instrumentation for ktor. Add library instrumentation for ktor server Dec 28, 2021
pipeline.environment.monitor.subscribe(Routing.RoutingCallStarted) { call ->
val context = call.attributes.getOrNull(contextKey)
if (context != null) {
ServerSpanNaming.updateServerSpanName(context, ServerSpanNaming.Source.SERVLET, { _, arg -> arg.route.parent.toString() }, call)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this set http.route attribute as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think so - do you think we should do it here or should ServerSpanName, or a similar helper, set them both?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we discussed this already somewhere... I don't remember our decision :(

Copy link
Member

Choose a reason for hiding this comment

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

http.route is going to be set by ServerSpanNaming (well, once I find enough time to implement that...)


internal val additionalExtractors = mutableListOf<AttributesExtractor<in ApplicationRequest, in ApplicationResponse>>()

internal var statusExtractor:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why is this a function and not just an instance of SpanStatusExtractor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the same pattern as many of the tracing builders. It's because it's very common to want to customize one specific case and then delegate to default for others.

pipeline.environment.monitor.subscribe(Routing.RoutingCallStarted) { call ->
val context = call.attributes.getOrNull(contextKey)
if (context != null) {
ServerSpanNaming.updateServerSpanName(context, ServerSpanNaming.Source.SERVLET, { _, arg -> arg.route.parent.toString() }, call)
Copy link
Member

Choose a reason for hiding this comment

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

http.route is going to be set by ServerSpanNaming (well, once I find enough time to implement that...)

@trask trask merged commit e08ed9d into open-telemetry:main Jan 5, 2022
RashmiRam pushed a commit to RashmiRam/opentelemetry-auto-instr-java that referenced this pull request May 23, 2022
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.

4 participants