Skip to content

Add Ktor client integration #4527

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

Open
wants to merge 30 commits into
base: main
Choose a base branch
from
Open

Add Ktor client integration #4527

wants to merge 30 commits into from

Conversation

lcian
Copy link
Member

@lcian lcian commented Jun 27, 2025

📜 Description

Add a Ktor client integration.
Functionality, options and tests are similar to our OkHttp integration.

💡 Motivation and Context

Part of #2684

💚 How did you test it?

End to end and manual tests

📝 Checklist

  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • I updated the wizard if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

  • Changelog
  • Docs
  • Readme
  • Add module to main readme
  • Release
  • Add module to .craft.yml
  • Add module to release registry
  • e2e tests

Copy link
Contributor

github-actions bot commented Jun 27, 2025

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 492.51 ms 542.98 ms 50.47 ms
Size 1.58 MiB 2.09 MiB 519.31 KiB

Previous results on branch: lcian/feat/ktor-client

Startup times

Revision Plain With Sentry Diff
c3b3ac8 433.74 ms 490.08 ms 56.34 ms
f316c06 427.39 ms 464.94 ms 37.55 ms
bc4c3ab 420.27 ms 462.92 ms 42.65 ms
64151fc 417.40 ms 436.90 ms 19.50 ms
94e993e 404.49 ms 439.62 ms 35.13 ms
1fa99ff 371.94 ms 406.67 ms 34.74 ms
df1fbd1 407.84 ms 424.93 ms 17.09 ms
ec32652 354.67 ms 385.71 ms 31.04 ms
1df63ce 429.52 ms 485.02 ms 55.50 ms
9647976 403.48 ms 420.36 ms 16.88 ms

App size

Revision Plain With Sentry Diff
c3b3ac8 1.58 MiB 2.09 MiB 518.90 KiB
f316c06 1.58 MiB 2.09 MiB 519.08 KiB
bc4c3ab 1.58 MiB 2.09 MiB 518.90 KiB
64151fc 1.58 MiB 2.09 MiB 519.31 KiB
94e993e 1.58 MiB 2.09 MiB 519.31 KiB
1fa99ff 1.58 MiB 2.09 MiB 519.27 KiB
df1fbd1 1.58 MiB 2.09 MiB 519.08 KiB
ec32652 1.58 MiB 2.09 MiB 518.89 KiB
1df63ce 1.58 MiB 2.09 MiB 518.90 KiB
9647976 1.58 MiB 2.09 MiB 519.31 KiB

@lcian lcian changed the title [WIP] Add Ktor client integration \Add Ktor client integration Jul 3, 2025
@lcian lcian changed the title \Add Ktor client integration Add Ktor client integration Jul 3, 2025
@lcian lcian marked this pull request as ready for review July 3, 2025 10:55
@lcian lcian force-pushed the lcian/feat/ktor-client branch from afb2f01 to 8b75851 Compare July 15, 2025 10:19
@lcian lcian force-pushed the lcian/feat/ktor-client branch from 7945398 to 8499b5d Compare July 15, 2025 12:02
Copy link
Member

@romtsn romtsn left a comment

Choose a reason for hiding this comment

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

LGTM, but there's one caveat we have to consider: If someone uses ktor with the OkHttp engine, we'd be potentially double-instrumenting the http calls, especially with SAGP in place. Could be a follow up PR, but I guess we have to add some protection against that case (like checking if engine == OkHttp and platform == Android then don't instrument OR retrieve the list of interceptors/eventListner from OkHttp client if possible, and check if ours are already there)

@lcian
Copy link
Member Author

lcian commented Jul 15, 2025

LGTM, but there's one caveat we have to consider: If someone uses ktor with the OkHttp engine, we'd be potentially double-instrumenting the http calls, especially with SAGP in place. Could be a follow up PR, but I guess we have to add some protection against that case (like checking if engine == OkHttp and platform == Android then don't instrument OR retrieve the list of interceptors/eventListner from OkHttp client if possible, and check if ours are already there)

Good call. Working on this and trying to keep the extra dependencies at a minimum. I think we'll need to bring in the library containing the OkHttp client engine at least, which probably also depends on OkHttp itself.

@lcian
Copy link
Member Author

lcian commented Jul 16, 2025

@sentry review

Copy link

PR Description

This pull request introduces a new integration for the Sentry error monitoring platform with the Ktor HTTP client. The goal is to automatically capture unhandled exceptions, create breadcrumbs for HTTP requests, and support distributed tracing for applications using Ktor's client.

Click to see more

Key Technical Changes

The key technical changes include: 1. Creation of a SentryKtorClientPlugin that implements Ktor's ClientPlugin interface. 2. Implementation of request and response interceptors to create spans and breadcrumbs. 3. Addition of a SentryKtorClientUtils class to handle error capturing and breadcrumb creation. 4. Implementation of SentryKtorClientPluginContextHook to manage scopes using SentryContext. 5. Addition of tests to verify the plugin's functionality.

Architecture Decisions

The architectural decisions include: 1. Using Ktor's ClientPlugin API to integrate with the HTTP client pipeline. 2. Using Sentry's existing IScopes and ISpan interfaces for managing scope and spans. 3. Implementing a context hook to ensure proper scope management within the Ktor pipeline. 4. Providing configuration options for capturing failed requests, defining target URLs, and customizing spans.

Dependencies and Interactions

This integration depends on the Ktor HTTP client library, the Sentry Java SDK, and the kotlinx.coroutines library. It interacts with the Sentry SDK to capture exceptions, create breadcrumbs, and manage spans. It also interacts with the Ktor client pipeline to intercept requests and responses.

Risk Considerations

Potential risks include: 1. Performance overhead due to span creation and breadcrumb generation. 2. Memory issues if response bodies are too large when capturing errors. 3. Thread safety issues if scopes are accessed concurrently. 4. Compatibility issues with future versions of Ktor or the Sentry SDK. 5. Incorrect configuration leading to excessive error reporting or missed errors.

Notable Implementation Details

Notable implementation details include: 1. The use of AttributeKey to store request start timestamps and spans within the Ktor request context. 2. The use of TracingUtils.traceIfAllowed to add Sentry trace headers to requests. 3. The implementation of captureClientError to create and capture Sentry events for failed HTTP requests. 4. The use of forceScopes for testing purposes to bypass the standard scope injection mechanism.

@lcian lcian force-pushed the lcian/feat/ktor-client branch from 28e039a to bb52a1b Compare July 17, 2025 13:03
@lcian lcian force-pushed the lcian/feat/ktor-client branch from 10fa401 to 8a42315 Compare July 17, 2025 13:26
@lcian
Copy link
Member Author

lcian commented Jul 17, 2025

@romtsn Please let me know what you think about this approach: 8a42315 (#4527)
As far as I can tell it's not possible to do this without reflection, as the config property has internal visibility.
I've also added a ProGuard rule that should ensure this works when ProGuard is being used, as we rely on those class/property names.

The approach is still not perfect because if the config block has some side effects, we would trigger them when creating the fake client.
However this seems to be the only approach to do this successfully.
Otherwise I guess another option would be to somehow detect the usage of OkHttp as engine for Ktor and conditionally disable the instrumentation in SAGP, and leave the responsibility to the user to not double instrument when not using SAGP.

As a next step I want to set up an app using ProGuard and SAGP to ensure this works as intended.

@lcian lcian requested a review from romtsn July 17, 2025 13:42
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