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

Rename ktor tracing to ktor telemetry #12855

Merged
merged 29 commits into from
Dec 17, 2024

Conversation

trask
Copy link
Member

@trask trask commented Dec 8, 2024

*KtorClientTracing* and *KtorServerTracing* have been deprecated and renamed to *KtorClientTelemetry* and *KtorServerTelemetry*

Also moves everthing under ktor-2-common into *.v2_0.common.* package

Part of #12846

See #12867 for change log migration notes entry

@github-actions github-actions bot added the test native This label can be applied to PRs to trigger them to run native tests label Dec 8, 2024
@trask trask force-pushed the rename-ktor-tracing branch 2 times, most recently from 0ef7bb4 to 252db08 Compare December 9, 2024 00:51
@trask trask force-pushed the rename-ktor-tracing branch 2 times, most recently from 3defa4a to f646ba2 Compare December 9, 2024 04:32
@trask trask force-pushed the rename-ktor-tracing branch from 4f5c848 to 04d72e2 Compare December 9, 2024 21:59
@trask trask marked this pull request as ready for review December 9, 2024 22:06
@trask trask requested a review from a team as a code owner December 9, 2024 22:06
@trask trask force-pushed the rename-ktor-tracing branch from 8b7b2f3 to 2c08222 Compare December 9, 2024 23:04
Copy link
Contributor

@breedx-splk breedx-splk left a comment

Choose a reason for hiding this comment

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

I see no issues with this. Very generous deprecation for a non-stable component. 😁

@trask trask force-pushed the rename-ktor-tracing branch from 69a8841 to 9397547 Compare December 10, 2024 04:18
fun setEmitExperimentalHttpClientMetrics(emitExperimentalHttpClientMetrics: Boolean) {
if (emitExperimentalHttpClientMetrics) {
emitExperimentalHttpClientMetrics()
}
}

@Deprecated("Please use method `Experimental.emitExperimentalHttpClientMetrics`")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is needed as the class itself is deprecated. Also doesn't Experimental only work with the new telemetry builder class?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was trying to provide a hint where this method went since it's not obvious otherwise

@github-actions github-actions bot requested a review from theletterf December 11, 2024 18:09
Comment on lines 30 to 31
internal lateinit var internalBuilder: DefaultHttpClientInstrumenterBuilder<HttpRequestData, HttpResponse>
protected lateinit var builder: DefaultHttpClientInstrumenterBuilder<HttpRequestData, HttpResponse>
Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't figure out how to have both internal and protected access

Copy link
Contributor

Choose a reason for hiding this comment

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

internal should be enough -- is there a reason why it shouldn't be public? It can only be used by other classes in this module when it's internal. 👍🏻

Copy link
Member Author

Choose a reason for hiding this comment

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

I found a better way than duplicating the field, I created an internal accessor to the protected field

@trask trask merged commit 9865c17 into open-telemetry:main Dec 17, 2024
56 checks passed
@trask trask deleted the rename-ktor-tracing branch December 17, 2024 03:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test native This label can be applied to PRs to trigger them to run native tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants