feat(ndk): add `traces_sample_rate` synchronization #1176

JoshuaMoelans · 2025-03-12T13:47:34Z

add option
write test(s) for the NDK <- done in this PR add NDK testing setup #1180
add to docs (on the Android side)
add to changelog
bump native + open up PR on sentry-Java (e.g. updating options in sentryNDK.java)
-> also have an integration test to make sure sending transactions works.

Since the NDK has no transport, we expect the Android side to pick up any envelopes (see this comment)

github-actions · 2025-03-12T13:48:02Z

	Fails
🚫	Please consider adding a changelog entry for the next release.

	Messages
📖	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- add `traces_sample_rate` synchronization ([#1176](https://github.com/getsentry/sentry-native/pull/1176))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against 031e949

JoshuaMoelans · 2025-03-12T14:06:17Z

ndk/sample/src/main/cpp/ndk-sample.cpp

@@ -22,4 +22,29 @@ JNIEXPORT void JNICALL Java_io_sentry_ndk_sample_NdkSample_message(JNIEnv *env,
    sentry_capture_event(event);
 }

+JNIEXPORT void JNICALL Java_io_sentry_ndk_sample_NdkSample_transaction(JNIEnv *env, jclass cls) {


Since the NDK has no transport, we can't really test this just by looking at Sentry dashboards/intercepting HTTP. A way to check if the envelope is created properly, is by looking at data/data/io.sentry.ndk.sample/files/outbox on the emulated device's storage.

This envelope-check is probably how we want to this test on the native repo (and then have a integration test similar to this one from native on the java-SDK side)

Yeah, this is fine. The outbox is the primary interface back to sentry-android and, as such, the ideal test boundary. Adding additional machinery beyond parsing the output won't provide better feedback.

ndk/sample/build.gradle.kts

JoshuaMoelans · 2025-03-24T15:24:37Z

back to draft, forgot to change the new variable to getter-setter only (it should not be part of constructor)

JoshuaMoelans · 2025-03-25T12:18:20Z

I'm not sure if there is a proper place to add this feature to the native docs, or if it makes more sense to only add it to the Android docs once we implement the feature to pass through the tracesSampleRate over there.

CHANGELOG.md

ndk/lib/src/androidTest/jni/ndk-test.cpp

supervacuus · 2025-03-25T13:40:40Z

I'm not sure if there is a proper place to add this feature to the native docs, or if it makes more sense to only add it to the Android docs once we implement the feature to pass through the tracesSampleRate over there.

Currently, this doesn't require much documentation. The ask is primarily for sentry-native to honor the setting from the sentry-android layer. Most people would expect it just to work, and it only pops up because it doesn't. Of course, the core question remains: Do we think it should work?

So, the getter/setter doesn't require attention from the docs. What should be documented, and probably on the Android side (but not necessarily by their team alone), is how transactions should work if enabled on both layers. This PR doesn't answer this since it only opens the pass-through down and back up (which is fine, but as such, it can't provide any clarifying insights either).

* Add dependencies to allow running unit tests as well as instrumented android tests * add ndk test into CI * moved NDK check+test into `test` job * add conditional check for ANDROID_API in NDK test/check jobs * trying to fix CI * trying to fix CI 2 * Add Java/Gradle setup steps * Apply checks to NDK lib only * Apply code formatting * run tests on reactivecircus emulator * remove KVM enable step * update CI (remove NDK test) --------- Co-authored-by: Markus Hintersteiner <[email protected]>

markushi

Looking good, nice! I left a few minor/medium comments, just ping me as soon as I should take a second look 🚀

ndk/lib/src/androidTest/java/io/sentry/ndk/sample/NdkSample.java

ndk/lib/src/androidTest/java/io/sentry/ndk/SentryNdkTest.java

ndk/lib/src/androidTest/java/io/sentry/ndk/sample/NdkSample.java

ndk/lib/src/test/java/io/sentry/ndk/NdkOptionsTest.java

ndk/sample/build.gradle.kts

ndk/lib/src/androidTest/jni/ndk-test.cpp

ndk/lib/src/androidTest/java/io/sentry/ndk/SentryNdkTest.java

ndk/lib/src/androidTest/java/io/sentry/ndk/sample/NdkTestHelper.java

ndk/lib/src/androidTest/jni/ndk-test.cpp

Co-authored-by: Mischan Toosarani-Hausberger <[email protected]>

markushi

LGTM!

ndk/lib/src/test/java/io/sentry/ndk/NdkOptionsTest.java

JoshuaMoelans commented Mar 12, 2025

View reviewed changes

JoshuaMoelans mentioned this pull request Mar 18, 2025

add NDK testing setup #1180

Merged

JoshuaMoelans marked this pull request as ready for review March 24, 2025 15:15

JoshuaMoelans requested review from markushi and supervacuus March 24, 2025 15:15

JoshuaMoelans marked this pull request as draft March 24, 2025 15:24

JoshuaMoelans marked this pull request as ready for review March 25, 2025 11:39

supervacuus requested changes Mar 25, 2025

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

ndk/lib/src/androidTest/jni/ndk-test.cpp Outdated Show resolved Hide resolved

JoshuaMoelans and others added 8 commits March 25, 2025 17:20

upgrade gradle

a253c54

add tracesSampleRate sync in NDK

cace44f

add tracing to NDK example

506ffcc

update CHANGELOG.md

d4de1eb

make tracesSampleRate a get/set instead of constructor parameter

6546ee0

add check for transaction type in test

53283e8

change log levels

f2cf32d

JoshuaMoelans force-pushed the joshua/feat/sync_android_tracessamplerate branch from 9bf3c1a to f2cf32d Compare March 25, 2025 16:32

update CHANGELOG.md

30cb025

markushi requested changes Mar 25, 2025

View reviewed changes

JoshuaMoelans added 3 commits March 26, 2025 10:07

apply suggestions from review

d872eab

add messageCaught function

485b5cb

remove CI testing artifact

e2a76eb

supervacuus requested changes Mar 26, 2025

View reviewed changes

JoshuaMoelans and others added 3 commits March 26, 2025 16:23

Apply suggestions from code review

08c1245

Co-authored-by: Mischan Toosarani-Hausberger <[email protected]>

suggestion from code review

7aab537

move NdkTestHelper

aa79dd5

supervacuus approved these changes Mar 26, 2025

View reviewed changes

remove unnecessary crash() function

d6b6ff4

JoshuaMoelans requested a review from markushi March 27, 2025 14:57

markushi approved these changes Mar 28, 2025

View reviewed changes

ndk/lib/src/test/java/io/sentry/ndk/NdkOptionsTest.java Outdated Show resolved Hide resolved

ndk/lib/src/test/java/io/sentry/ndk/NdkOptionsTest.java Outdated Show resolved Hide resolved

cleanup tests

031e949

JoshuaMoelans merged commit 20d5d5f into master Mar 28, 2025
30 checks passed

JoshuaMoelans deleted the joshua/feat/sync_android_tracessamplerate branch March 28, 2025 10:10

BrewTestBot mentioned this pull request Mar 28, 2025

sentry-native 0.8.3 Homebrew/homebrew-core#216954

Merged

1 task

Uh oh!

feat(ndk): add traces_sample_rate synchronization #1176

feat(ndk): add traces_sample_rate synchronization #1176

Conversation

JoshuaMoelans commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Instructions and example for changelog

Uh oh!

JoshuaMoelans Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

supervacuus Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JoshuaMoelans commented Mar 24, 2025

Uh oh!

JoshuaMoelans commented Mar 25, 2025

Uh oh!

Uh oh!

Uh oh!

supervacuus commented Mar 25, 2025

Uh oh!

markushi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

markushi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

feat(ndk): add `traces_sample_rate` synchronization #1176

feat(ndk): add `traces_sample_rate` synchronization #1176

JoshuaMoelans commented Mar 12, 2025 •

edited

Loading

github-actions bot commented Mar 12, 2025 •

edited

Loading

JoshuaMoelans Mar 12, 2025 •

edited

Loading