fix: Apply Enricher to User Feedback
#4883
Conversation
bitsandfoxes
requested review from
Flash0ver and
jamescrosswell
as code owners
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4883 +/- ##
==========================================
- Coverage 73.84% 73.83% -0.01%
==========================================
Files 483 483
Lines 17577 17578 +1
Branches 3464 3464
==========================================
- Hits 12979 12978 -1
- Misses 3742 3743 +1
- Partials 856 857 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // Act | ||
| sut.CaptureFeedback(feedback); | ||
|
|
||
| // Assert | ||
| var item = envelope.Items.First(x => x.TryGetType() == EnvelopeItem.TypeValueFeedback); |
There was a problem hiding this comment.
suggestion: assert result
Would you mind also asserting the resulting ID not to be SentryId.Empty, for consistency with the other CaptureFeedback tests.
| // Act | |
| sut.CaptureFeedback(feedback); | |
| // Assert | |
| var item = envelope.Items.First(x => x.TryGetType() == EnvelopeItem.TypeValueFeedback); | |
| // Act | |
| var result = sut.CaptureFeedback(feedback); | |
| // Assert | |
| result.Should().NotBe(SentryId.Empty); | |
| var item = envelope.Items.First(x => x.TryGetType() == EnvelopeItem.TypeValueFeedback); |
| Replace your usage of `WithScope` with overloads of `Capture*` methods: | ||
|
|
||
| - `SentrySdk.CaptureEvent(SentryEvent @event, Action<Scope> scopeCallback)` |
There was a problem hiding this comment.
praise: thanks for the cleanup 🥔
| // Evaluate and copy before invoking the callback | ||
| scope.Evaluate(); | ||
| scope.Apply(evt); | ||
| _enricher.Apply(evt); |
There was a problem hiding this comment.
question: more than environment and release
More than environment and release are applied ... is this intended?
| scope.Apply(evt); | ||
| _enricher.Apply(evt); |
There was a problem hiding this comment.
question: precedence
Is the order of first applying the Scope, and then the Event-Enricher correct?
Or should instead e.g. Tags on the Scope overwrite Default-Tags from the Enricher?
On the other hand, this question would then similarly apply to e.g. Transactions, too, wouldn't it?
And changing the order the could be considered a behavioral breaking change ... or maybe a fix?
On the other hand ... we could just let "local consistency" win and do it as you suggested.
What are your thoughts?
Resolves getsentry/sentry-unity#2510
Without the enricher, User Feedback is lacking
environmentandrelease.