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 GraphQL Apollo Kotlin 4 integration #4166

Merged
merged 12 commits into from
Feb 24, 2025
Merged

Add GraphQL Apollo Kotlin 4 integration #4166

merged 12 commits into from
Feb 24, 2025

Conversation

lcian
Copy link
Member

@lcian lcian commented Feb 14, 2025

📜 Description

Adds a new module to integrate Apollo Kotlin 4.

💡 Motivation and Context

Closes #3662

💚 How did you test it?

Unit tests with both ApolloCall<D>::execute and ApolloCall<D>::executeV3 (behaving as v3).

📝 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.

@lcian
Copy link
Member Author

lcian commented Feb 14, 2025

NOTE: to review this I would recommend just looking at the diff between the first commit in the PR and the last one, the first commit is porting the code we have for v3 to v4
i.e. use this to review: https://github.com/getsentry/sentry-java/pull/4166/files/4fac45da4ca55a72121135b0b641c209da0a4661..59977875b26d512fa23f365827ec5f1b8077e367

Copy link
Contributor

github-actions bot commented Feb 14, 2025

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 392.31 ms 455.23 ms 62.92 ms
Size 1.58 MiB 2.21 MiB 641.48 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
c2c78de 415.28 ms 505.08 ms 89.80 ms

App size

Revision Plain With Sentry Diff
c2c78de 1.58 MiB 2.21 MiB 640.27 KiB

Previous results on branch: lcian/feat/apollo-4

Startup times

Revision Plain With Sentry Diff
b6f49c8 347.02 ms 419.10 ms 72.08 ms
afa0712 379.70 ms 463.00 ms 83.30 ms
98d90f9 410.86 ms 515.15 ms 104.29 ms

App size

Revision Plain With Sentry Diff
b6f49c8 1.58 MiB 2.21 MiB 641.11 KiB
afa0712 1.58 MiB 2.21 MiB 641.06 KiB
98d90f9 1.58 MiB 2.21 MiB 641.09 KiB

@lcian lcian marked this pull request as ready for review February 14, 2025 12:42
@lcian lcian requested a review from lbloder February 17, 2025 11:31
Copy link
Member

@markushi markushi left a comment

Choose a reason for hiding this comment

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

NOTE: to review this I would recommend just looking at the diff between the first commit in the PR and the last one, the first commit is porting the code we have for v3 to v4
i.e. use this to review: 4fac45da..59977875 (#4166)

I mainly reviewed exactly that, looks good to me! Do you think we should release this in an alpha version first?

@lcian
Copy link
Member Author

lcian commented Feb 19, 2025

I mainly reviewed exactly that, looks good to me! Do you think we should release this in an alpha version first?

Thanks @markushi !
I've tested this manually too and I don't see any way this could break customer's code, so I think we could release it in a regular version. Please let me know if you think otherwise.

@adinauer
Copy link
Member

Since this adds a new module and doesn't change existing code I think it's safe to release without an alpha as the only customers affected are those explicitly using the new Apollo 4 dependency and actively configuring it to be used.

@adinauer
Copy link
Member

Deferring to @lbloder for a review here since he wrote the integration for v3.

Copy link
Collaborator

@lbloder lbloder left a comment

Choose a reason for hiding this comment

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

LGTM, nicely done 👍

To make the CI run successfully, you'll need to run make api once locally and push the changes.

@lcian lcian changed the title Add Apollo 4 integration Add GraphQL Apollo Kotlin 4 integration Feb 24, 2025
@lcian
Copy link
Member Author

lcian commented Feb 24, 2025

Thanks for kickstarting this @cvb941 !

@lcian lcian merged commit c87d429 into main Feb 24, 2025
35 checks passed
@lcian lcian deleted the lcian/feat/apollo-4 branch February 24, 2025 12:35
@lcian lcian mentioned this pull request Feb 24, 2025
6 tasks
@cvb941
Copy link
Contributor

cvb941 commented Feb 24, 2025

thanks @lcian for the work here, I hope many people will use the updated Apollo integration as well 😊

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.

Support Apollo4
5 participants