Skip to content

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

@github-actions
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

6 participants