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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only send userid in Dynamic Sampling Context if sendDefaultPii is true #2147

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Unreleased

### Fixes

- Only send userid in Dynamic Sampling Context if sendDefaultPii is true ([#2147](https://github.com/getsentry/sentry-java/pull/2147))

### Features

- New package `sentry-android-navigation` for AndroidX Navigation support ([#2136](https://github.com/getsentry/sentry-java/pull/2136))
Expand Down
11 changes: 10 additions & 1 deletion sentry/src/main/java/io/sentry/TraceContext.java
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,21 @@ public final class TraceContext implements JsonUnknown, JsonSerializable {
new Dsn(sentryOptions.getDsn()).getPublicKey(),
sentryOptions.getRelease(),
sentryOptions.getEnvironment(),
user != null ? user.getId() : null,
getUserId(sentryOptions, user),
user != null ? getSegment(user) : null,
transaction.getName(),
sampleRateToString(sampleRate(samplingDecision)));
}

private static @Nullable String getUserId(
final @NotNull SentryOptions options, final @Nullable User user) {
if (options.isSendDefaultPii() && user != null) {
return user.getId();
}

return null;
}

private static @Nullable String getSegment(final @NotNull User user) {
final Map<String, String> others = user.getOthers();
if (others != null) {
Expand Down
81 changes: 81 additions & 0 deletions sentry/src/test/java/io/sentry/SentryTracerTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -481,6 +481,7 @@ class SentryTracerTest {
fun `returns trace state`() {
val transaction = fixture.getSut({
it.isTraceSampling = true
it.isSendDefaultPii = true
})
fixture.hub.setUser(
User().apply {
Expand All @@ -500,6 +501,29 @@ class SentryTracerTest {
}
}

@Test
fun `returns trace state without userId if not send pii`() {
Copy link
Member

Choose a reason for hiding this comment

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

l: For full conditional coverage you could also add a test if the user is null and isSendDefaultPii = true, but maybe that's overkill. Up to you.

Copy link
Member Author

Choose a reason for hiding this comment

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

added returns baggage header without userId if send pii and null user below

val transaction = fixture.getSut({
it.isTraceSampling = true
})
fixture.hub.setUser(
User().apply {
id = "user-id"
others = mapOf("segment" to "pro")
}
)
val trace = transaction.traceContext()
assertNotNull(trace) {
assertEquals(transaction.spanContext.traceId, it.traceId)
assertEquals("key", it.publicKey)
assertEquals("environment", it.environment)
assertEquals("release@3.0.0", it.release)
assertEquals(transaction.name, it.transaction)
assertNull(it.userId)
assertEquals("pro", it.userSegment)
}
}

@Test
fun `trace state does not change once computed`() {
val transaction = fixture.getSut({
Expand All @@ -525,6 +549,7 @@ class SentryTracerTest {
it.isTraceSampling = true
it.environment = "production"
it.release = "1.0.99-rc.7"
it.isSendDefaultPii = true
})

fixture.hub.setUser(
Expand All @@ -549,6 +574,62 @@ class SentryTracerTest {
}
}

@Test
fun `returns baggage header without userId if not send pii`() {
val transaction = fixture.getSut({
it.isTraceSampling = true
it.environment = "production"
it.release = "1.0.99-rc.7"
})

fixture.hub.setUser(
User().apply {
id = "userId12345"
others = mapOf("segment" to "pro")
}
)

val header = transaction.toBaggageHeader()
assertNotNull(header) {
assertEquals("baggage", it.name)
assertNotNull(it.value)
println(it.value)
assertTrue(it.value.contains("sentry-trace_id=[^,]+".toRegex()))
assertTrue(it.value.contains("sentry-public_key=key,"))
assertTrue(it.value.contains("sentry-release=1.0.99-rc.7,"))
assertTrue(it.value.contains("sentry-environment=production,"))
assertTrue(it.value.contains("sentry-transaction=name,"))
assertFalse(it.value.contains("sentry-user_id"))
assertTrue(it.value.contains("sentry-user_segment=pro$".toRegex()))
}
}

@Test
fun `returns baggage header without userId if send pii and null user`() {
val transaction = fixture.getSut({
it.isTraceSampling = true
it.environment = "production"
it.release = "1.0.99-rc.7"
it.isSendDefaultPii = true
})

fixture.hub.setUser(null)

val header = transaction.toBaggageHeader()
assertNotNull(header) {
assertEquals("baggage", it.name)
assertNotNull(it.value)
println(it.value)
assertTrue(it.value.contains("sentry-trace_id=[^,]+".toRegex()))
assertTrue(it.value.contains("sentry-public_key=key,"))
assertTrue(it.value.contains("sentry-release=1.0.99-rc.7,"))
assertTrue(it.value.contains("sentry-environment=production,"))
assertTrue(it.value.contains("sentry-transaction=name"))
assertFalse(it.value.contains("sentry-user_id"))
assertFalse(it.value.contains("sentry-user_segment"))
}
}

@Test
fun `sets ITransaction data as extra in SentryTransaction`() {
val transaction = fixture.getSut(samplingDecision = TracesSamplingDecision(true))
Expand Down