Fix: Apply user from the scope to transaction. #1424

maciejwalkowiak · 2021-04-19T10:26:01Z

📜 Description

Apply user from the scope to transaction.

💡 Motivation and Context

#1335 Note: this does not solve the issue of setting user on transaction in Spring/Spring Boot integrations.

💚 How did you test it?

Unit tests.

📝 Checklist

I reviewed the submitted code
I added tests to verify the changes
I updated the docs if needed
No breaking changes

Adding nullability annotations to user makes it a breaking change for Kotlin users.

🔮 Next steps

Fix setting user on transaction for Spring & Spring Boot.

sentry/src/main/java/io/sentry/SentryBaseEvent.java

marandaneto · 2021-04-19T11:12:50Z

sentry-spring/src/test/kotlin/io/sentry/spring/SentryUserProviderEventProcessorTest.kt

+        assertNotNull(result.user) {
+            assertEquals("john.doe", it.username)
+            assertEquals("user-id", it.id)
+            assertEquals("192.168.0.1", it.ipAddress)
+            assertEquals("john.doe@example.com", it.email)
+            assertEquals(mapOf("key" to "value"), it.others)
+        }


this block is duplicated across tests, maybe we extract it to a method called asserUser or something like that?

I think in case of this test class duplicated code is a good trade off. Having fixed string values in assertUser method although resolves duplication, doesn't feel right to couple this method with test methods. Feel free to refactor it if you like.

tbh I'm not a big fan of that, it's the same strings and same asserts across every test, every time you'd need to change either a value or adding/removing a field, you've to change it everywhere.
I know @philipphofmann has strong opinions about that too, it's not scalable when the number of tests increases.

I agree 100% with @marandaneto. Treat your test code as production code. A few duplications are fine, but in this case, I would extract them to a method.

I will merge as it is because I am blocked by this PR, but lets please discuss and refactor this as soon as we touch this class again

I'm not willing to die on this hill so bare with me:

I believe a test should be as self-sufficient as possible. When a test calls out to a private method getUser then assertUser it isn't clear what user is, and how assert works, unless I peek into those methods. It also means that I can be setting the user now thinking of a particular test, but be negatively affecting another, without knowing, since multiple tests "change" by means of modifying these common functions.

It has the advantage, obviously, of having a single place to "fix" things, or even to add a new property to be set and asserted. It But that comes with the cost of potentially changing a test that wasn't supposed to be changed.

Perhaps what we're missing here is a library that validates two objects are "equivalent". It solves the problem of adding a new property to the field and also expecting that to be mapped through which is a common mistake when mapping objects by hand.

Both these principles/approaches are used in the .NET SDK. The assertion library (introduced by @Tyrrrz, who from what I noticed also follows the idea of self-sufficient tests) is called FluentAssertions , is there something similar for Java?

All of that said, this class in particular does have a lot of duplication for basic object mapping, to the point that this diff was mainly changes to asserting user. I'd be very tempted to at least create a assertUserEquals(User a, User b) to get rid of this duplication.

Kotlin generates equals and hashCode for data classes, but Java does not, we'd need to implement and bloat the SDK with that (which is gonna be used only in tests), so we could do an equality check.

another approach is having a test per field, instead of the whole user every time.

I do agree that's a trade-off, but I often spend more time scaling the tests other than "understanding" them.

marandaneto · 2021-04-19T11:16:49Z

sentry/src/main/java/io/sentry/SentryClient.java

+      if (sentryBaseEvent.getUser() == null) {
+        sentryBaseEvent.setUser(scope.getUser());
+      }


what's about the combination with this check and MainEventProcessor? do they conflict with each other when isSendDefaultPii is enabled? I believe one of them will never be executed if it sets to a non-null value

If user is not set on the event, SentryClient sets it from the scope. Then in MainEventProcessor if user is null it sets the user with a default ip address, if its not null but the ip address is null it sets the default ip address. What conflict do you mean?

indeed, apply scope happens before event processors, that's why, the other way around would be problematic I believe.

a problem that I see is, MainEventProcessor takes into consideration the isSendDefaultPii and sets IP address with DEFAULT_IP_ADDRESS, but that won't happen for transactions, this can be fixed later IMO when we run event processors for transactions too.

We could start setting the default value even if a user is set, but the IpAddress is null.
I don't believe the "setting user semantics" dictates it's a all or nothing thing, or does it?

Can't we merge values instead?

If I do captureEvent(Event{ user = User(name)) while having sendDefaultPii=true I'd still like the the {{auto}} IP Address to be added to the user. I just set the name because that's all I have.

marandaneto · 2021-04-19T11:18:16Z

@maciejwalkowiak this PR cannot be released alone though, if it reads from Scope but integrations don't set the user request to the Scope, Scope.user will be always null

maciejwalkowiak · 2021-04-19T11:23:49Z

@marandaneto for anyone who is not using integrations but does manual instrumentation this PR does provide a value. As I understand no matter how we solve the integrations, scope should be applied to transactions anyways.

marandaneto · 2021-04-19T11:40:55Z

@marandaneto for anyone who is not using integrations but does manual instrumentation this PR does provide a value. As I understand no matter how we solve the integrations, scope should be applied to transactions anyways.

I agree, but ideally this PR or another one should fix for our integrations too, see #1335 (comment)

marandaneto · 2021-04-19T12:55:17Z

what we could do is: creating the Spring filter that sets the user to the Scope and add it to https://docs.sentry.io/platforms/java/guides/spring/performance/#enable-tracing
so the error tracking works as it is, but the Spring (without spring boot) requires this additional step to fix the issue.

wdyt @maciejwalkowiak @bruno-garcia ?

maciejwalkowiak · 2021-04-19T13:58:15Z

@marandaneto yes I agree. Please note, that adding extra filter for Spring integration is not as big problem as it may look. Using Spring without Spring Boot, means that users must configure a lot of things manually, and while it is possible to configure some things that happen inside Spring context (like @EnableSentry does), it's idiomatic to add filters manually.

To simplify user experience we could have just a single filter that does everything, but then the performance measurement would not start as early as it is right now.

codecov-commenter · 2021-04-19T14:18:37Z

Codecov Report

Merging #1424 (f003a37) into main (e905c4b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##               main    #1424   +/-   ##
=========================================
  Coverage     75.79%   75.80%           
- Complexity     1865     1866    +1     
=========================================
  Files           185      185           
  Lines          6387     6389    +2     
  Branches        636      637    +1     
=========================================
+ Hits           4841     4843    +2     
  Misses         1258     1258           
  Partials        288      288

Impacted Files	Coverage Δ	Complexity Δ
sentry/src/main/java/io/sentry/SentryEvent.java	`68.88% <ø> (-1.01%)`	`40.00 <0.00> (-2.00)`
...entry/src/main/java/io/sentry/SentryBaseEvent.java	`85.41% <100.00%> (+0.97%)`	`25.00 <2.00> (+2.00)`
sentry/src/main/java/io/sentry/SentryClient.java	`88.44% <100.00%> (+0.07%)`	`85.00 <0.00> (+1.00)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e905c4b...f003a37. Read the comment docs.

bruno-garcia

👍
Though as Manoel pointed out, we'll miss the default IP address at times. Left a note on that

bruno-garcia · 2021-04-20T17:01:34Z

sentry/src/main/java/io/sentry/SentryClient.java

+      if (sentryBaseEvent.getUser() == null) {
+        sentryBaseEvent.setUser(scope.getUser());
+      }


We could start setting the default value even if a user is set, but the IpAddress is null.
I don't believe the "setting user semantics" dictates it's a all or nothing thing, or does it?

Can't we merge values instead?

If I do captureEvent(Event{ user = User(name)) while having sendDefaultPii=true I'd still like the the {{auto}} IP Address to be added to the user. I just set the name because that's all I have.

maciejwalkowiak · 2021-04-20T22:05:21Z

@bruno-garcia the question is should we lift setting the IP address from MainEventProcessor to SentryClient or wait till event processors support transactions?

marandaneto · 2021-04-21T07:32:01Z

@bruno-garcia the question is should we lift setting the IP address from MainEventProcessor to SentryClient or wait till event processors support transactions?

let's not block this PR because of that, here is the issue to follow up later when we execute event processors #1427

marandaneto · 2021-04-21T07:33:35Z

...-spring-boot-starter/src/test/kotlin/io/sentry/spring/boot/it/SentrySpringIntegrationTest.kt

                assertThat(event.user).isNotNull()
-                assertThat(event.user.username).isEqualTo("user")
-                assertThat(event.user.ipAddress).isEqualTo("169.128.0.1")
+                assertThat(event.user!!.username).isEqualTo("user")
+                assertThat(event.user!!.ipAddress).isEqualTo("169.128.0.1")


maybe use assertNotNull with the lambda so we avoid !! for all user asserts on this PR?

github-actions · 2021-04-21T08:28:26Z

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

`Instructions and example for changelog`$

Please add an entry to CHANGELOG.md` to the "Unreleased" section under the following heading:

To the changelog entry, please add a link to this PR (consider a more descriptive message):`

- Apply user from the scope to transaction(#1424)

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

Generated by 🚫 dangerJS against 86c0a40

maciejwalkowiak added 2 commits April 19, 2021 11:34

Apply user from the scope to transaction.

de41eb8

Changelog.

a79f6a3

maciejwalkowiak marked this pull request as ready for review April 19, 2021 10:26

maciejwalkowiak requested a review from marandaneto as a code owner April 19, 2021 10:26

Fix Android test.

ddd27b1

marandaneto reviewed Apr 19, 2021

View reviewed changes

sentry/src/main/java/io/sentry/SentryBaseEvent.java Outdated Show resolved Hide resolved

marandaneto reviewed Apr 19, 2021

View reviewed changes

Add nullability annotation.

f003a37

marandaneto requested a review from bruno-garcia April 19, 2021 20:14

bruno-garcia approved these changes Apr 20, 2021

View reviewed changes

marandaneto mentioned this pull request Apr 21, 2021

Merge user IP with {{auto}} if IP is null and sendDefaultPii is enabled, for events and transactions #1427

Closed

marandaneto reviewed Apr 21, 2021

View reviewed changes

Merge branch 'main' into gh-1335-2

86c0a40

marandaneto merged commit 7ac36c0 into main Apr 21, 2021

marandaneto deleted the gh-1335-2 branch April 21, 2021 08:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Apply user from the scope to transaction. #1424

Fix: Apply user from the scope to transaction. #1424

maciejwalkowiak commented Apr 19, 2021

marandaneto Apr 19, 2021

maciejwalkowiak Apr 20, 2021

marandaneto Apr 21, 2021

philipphofmann Apr 21, 2021

marandaneto Apr 21, 2021

bruno-garcia Apr 21, 2021

marandaneto Apr 21, 2021

marandaneto Apr 19, 2021

maciejwalkowiak Apr 19, 2021

marandaneto Apr 19, 2021

marandaneto Apr 19, 2021

bruno-garcia Apr 20, 2021

marandaneto commented Apr 19, 2021

maciejwalkowiak commented Apr 19, 2021

marandaneto commented Apr 19, 2021

marandaneto commented Apr 19, 2021

maciejwalkowiak commented Apr 19, 2021

codecov-commenter commented Apr 19, 2021

bruno-garcia left a comment

bruno-garcia Apr 20, 2021

maciejwalkowiak commented Apr 20, 2021

marandaneto commented Apr 21, 2021

marandaneto Apr 21, 2021

github-actions bot commented Apr 21, 2021

Fix: Apply user from the scope to transaction. #1424

Fix: Apply user from the scope to transaction. #1424

Conversation

maciejwalkowiak commented Apr 19, 2021

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marandaneto commented Apr 19, 2021

maciejwalkowiak commented Apr 19, 2021

marandaneto commented Apr 19, 2021

marandaneto commented Apr 19, 2021

maciejwalkowiak commented Apr 19, 2021

codecov-commenter commented Apr 19, 2021

Codecov Report

bruno-garcia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maciejwalkowiak commented Apr 20, 2021

marandaneto commented Apr 21, 2021

Choose a reason for hiding this comment

github-actions bot commented Apr 21, 2021