feat(feedback): initial release #4874

armcknight · 2025-02-19T23:37:03Z

Publicly exposes the feature. For #4709

TODO:

deprecate old api
Docs PR: Cocoa User Feedback sentry-docs#11191

github-actions · 2025-02-19T23:37:33Z

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

Generated by 🚫 dangerJS against 5024f37

codecov · 2025-02-19T23:44:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.310%. Comparing base (2ef90a3) to head (5024f37).
Report is 4 commits behind head on armcknight/feat(feedback)/final-fixes.

Additional details and impacted files

@@                             Coverage Diff                              @@
##           armcknight/feat(feedback)/final-fixes     #4874        +/-   ##
============================================================================
+ Coverage                                  9.094%   92.310%   +83.215%     
============================================================================
  Files                                        350       659       +309     
  Lines                                      24970     77407     +52437     
  Branches                                      94     28024     +27930     
============================================================================
+ Hits                                        2271     71455     +69184     
+ Misses                                     22699      5853     -16846     
- Partials                                       0        99        +99

Files with missing lines	Coverage Δ
SentryTestUtils/TestClient.swift	`85.714% <100.000%> (ø)`
SentryTestUtils/TestTransportAdapter.swift	`84.210% <ø> (ø)`
Sources/Sentry/SentryClient.m	`98.309% <100.000%> (+88.587%)`	⬆️
Sources/Sentry/SentryTransportAdapter.m	`100.000% <ø> (+83.076%)`	⬆️
Sources/Sentry/SentryUserFeedback.m	`100.000% <ø> (+100.000%)`	⬆️
...ift/Integrations/UserFeedback/SentryFeedback.swift	`72.727% <100.000%> (+72.727%)`	⬆️
...ts/Integrations/Feedback/SentryFeedbackTests.swift	`97.633% <100.000%> (ø)`
...tryTests/Networking/SentryHttpTransportTests.swift	`97.250% <100.000%> (ø)`
...Tests/Networking/SentryTransportAdapterTests.swift	`100.000% <ø> (ø)`
...sts/SentryTests/Protocol/SentryEnvelopeTests.swift	`96.024% <100.000%> (ø)`
... and 4 more

... and 637 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

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

github-actions · 2025-02-19T23:48:42Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1227.55 ms	1244.76 ms	17.20 ms
Size	22.31 KiB	821.70 KiB	799.39 KiB

Baseline results on branch: armcknight/feat(feedback)/final-fixes

Startup times

Revision	Plain	With Sentry	Diff
`f09e892`	1220.22 ms	1238.80 ms	18.57 ms
`38ae188`	1221.04 ms	1246.91 ms	25.87 ms
`cc14e49`	1209.31 ms	1231.54 ms	22.24 ms

App size

Revision	Plain	With Sentry	Diff
`f09e892`	22.31 KiB	820.98 KiB	798.67 KiB
`38ae188`	22.31 KiB	820.94 KiB	798.63 KiB
`cc14e49`	22.31 KiB	821.57 KiB	799.26 KiB

Previous results on branch: armcknight/feat(feedback)/initial-release

Startup times

Revision	Plain	With Sentry	Diff
`331f1be`	1197.30 ms	1229.26 ms	31.96 ms
`588c9d0`	1243.65 ms	1258.61 ms	14.96 ms
`2b6f0d6`	1221.55 ms	1234.17 ms	12.62 ms

App size

Revision	Plain	With Sentry	Diff
`331f1be`	22.31 KiB	821.88 KiB	799.56 KiB
`588c9d0`	22.31 KiB	821.59 KiB	799.28 KiB
`2b6f0d6`	22.31 KiB	821.70 KiB	799.39 KiB

This reverts commit ec5c3cd.

… old

Tests/SentryTests/SentryClientTests.swift

armcknight · 2025-02-20T23:32:16Z

Tests/SentryTests/SentryClientTests.swift

+    func testCaptureFeedback_WithEmptyEventId() {
+        let sut = fixture.getSut()
+        XCTAssertTrue(fixture.transportAdapter.sendEventWithTraceStateInvocations.isEmpty)
+        sut.capture(feedback: fixture.feedback, scope: fixture.scope)
+        XCTAssertFalse(fixture.transportAdapter.sendEventWithTraceStateInvocations.isEmpty)


@philipphofmann LMK what you think of this test. I debugged and traced execution to something I could check, which turned out to be the transport adapter, so just used that. I do see other test cases using sendEventWithTraceStateInvocations as well.

I think it's OK to do it like that, and the test should assert some properties of the recorded event. Now it only asserts that there was an event recorded but not that the contents of the event are correct.

philprime

Left a couple of comments, but other than that LGTM

philprime · 2025-02-21T09:43:03Z

Sources/Sentry/Public/SentryOptions.h

+ * submits calling the new method @c SentrySDK.captureFeedback (see
+ * https://docs.sentry.io/platforms/apple/user-feedback/configuration/).
+ */
+@property (nonatomic, copy, nullable)


l: Is there any value in not configuring this in the root options, but instead moving it to something like options.userFeedback.configure { ... }, implementing the nested options in Swift instead? Similar to the experimental options

philprime · 2025-02-21T09:47:02Z

Sources/Swift/Integrations/UserFeedback/SentryFeedback.swift

@@ -56,7 +56,7 @@ extension SentryFeedback: SentrySerializable {
        if let associatedEventId = associatedEventId {
            dict["associated_event_id"] = associatedEventId.sentryIdString
        }
-        dict["source"] = source.rawValue
+        dict["source"] = source.description


l: Just want to point out that in most cases it is bad practice to use description in any serialization, because if the implementation of description in line 7 is removed, the Swift compiler could still synthesize the CustomStringConvertible, and it's not guaranteed to match the raw string value (could include information like Optional<String>)

philprime · 2025-02-21T09:48:35Z

Tests/SentryTests/Integrations/Feedback/SentryFeedbackTests.swift

+        let sut = SentryFeedback(message: "Test feedback message", name: "Test feedback provider", email: "[email protected]", source: .custom, attachments: [Data()])
+
+        let serialization = sut.serialize()
+        XCTAssertEqual(try XCTUnwrap(serialization["message"] as? String), "Test feedback message")


l: Why the nesting of XCTUnwrap and XCTAssertEqual? AFAIK the XCTUnwrap is unnecessary, because we can compare serialization["message"] as? String directly to Test feedback message

philipphofmann

LGTM, with a couple of things to improve, but nothing blocking.

philipphofmann · 2025-02-21T15:06:39Z

Samples/iOS-ObjectiveC/iOS-ObjectiveC/ViewController.m

@@ -78,6 +78,28 @@ - (IBAction)captureUserFeedback:(id)sender
    [SentrySDK captureUserFeedback:userFeedback];
 }

+- (IBAction)captureNewUserFeedback:(id)sender


l: Using V2 instead of new for naming is usually better. What if we have user feedback V3? Are we going to call it newnew then?

oh yeah, we talked about this before with the profiling stuff 🤣 I'll change it to V2

Sources/Sentry/Public/SentryClient.h

philipphofmann · 2025-02-21T15:10:03Z

Sources/Sentry/Public/SentryOptions.h

+typedef void (^SentryUserFeedbackConfigurationBlock)(
+    SentryUserFeedbackConfiguration *_Nonnull configuration);


l: We have all the other typedefs for the callbacks in the options in SentryDefines.h. You could move it there for consistency.

philipphofmann · 2025-02-21T15:10:50Z

Sources/Sentry/Public/SentryOptions.h

+ * @note This is unrelated to @c SentrySDK.captureUserFeedback which is the old method of submitting
+ * user feedback you've already gathered via your own UI


m: I would write about deprecated instead of old.

Suggested change

* @note This is unrelated to @c SentrySDK.captureUserFeedback which is the old method of submitting

* user feedback you've already gathered via your own UI

* @note This is unrelated to @c SentrySDK.captureUserFeedback , which is the deprecated method of submitting

* user feedback you've already gathered via your own UI

philipphofmann · 2025-02-21T15:11:18Z

Sources/Sentry/Public/SentryOptions.h

+ * (see https://docs.sentry.io/platforms/apple/user-feedback/#user-feedback-api). The new method
+ * uses either this block to configure and a widget and UI form to gather feedback, or directly


l:

Suggested change

* (see https://docs.sentry.io/platforms/apple/user-feedback/#user-feedback-api). The new method

* uses either this block to configure and a widget and UI form to gather feedback, or directly

* (see https://docs.sentry.io/platforms/apple/user-feedback/#user-feedback-api). This method

* uses either this block to configure and a widget and UI form to gather feedback, or directly

philipphofmann · 2025-02-21T15:16:28Z

Tests/SentryTests/SentryClientTests.swift

+        assertNothingSent()
+    }
+
+    @available(*, deprecated, message: "This is only marked as deprecated because assertNothingSent is marked as deprecated, due to it using a deprecated property inside it. When that property usage is removed, this deprecation annotation can go away.")


Oh my god this repetition is very annoying 🤦. We will fix it later.

philipphofmann · 2025-02-21T15:21:48Z

Tests/SentryTests/SentryClientTests.swift

+    func testCaptureFeedback_WithEmptyEventId() {
+        let sut = fixture.getSut()
+        XCTAssertTrue(fixture.transportAdapter.sendEventWithTraceStateInvocations.isEmpty)
+        sut.capture(feedback: fixture.feedback, scope: fixture.scope)
+        XCTAssertFalse(fixture.transportAdapter.sendEventWithTraceStateInvocations.isEmpty)


I think it's OK to do it like that, and the test should assert some properties of the recorded event. Now it only asserts that there was an event recorded but not that the contents of the event are correct.

armcknight requested review from philipphofmann and philprime as code owners February 19, 2025 23:37

armcknight changed the base branch from main to armcknight/feat(feedback)/final-fixes February 19, 2025 23:37

armcknight force-pushed the armcknight/feat(feedback)/initial-release branch from 4957680 to e474fea Compare February 20, 2025 03:01

armcknight added 5 commits February 19, 2025 18:16

WIP: expose necessary interfaces publicly

05f6fb5

changelog

c808ffa

add screenshot to attach via the manual call to submit feedback

8a44552

Revert "WIP: we need to make these changes in the public release pr"

452f30f

This reverts commit ec5c3cd.

use the new api

5957494

armcknight force-pushed the armcknight/feat(feedback)/initial-release branch from e474fea to 5957494 Compare February 20, 2025 03:16

Format code

35176f9

armcknight marked this pull request as draft February 20, 2025 08:32

armcknight mentioned this pull request Feb 20, 2025

feedback: First shipment to public as early access / beta #4709

Open

5 tasks

armcknight and others added 11 commits February 20, 2025 10:26

fix test failure and add some missing ones

aaa25cf

actually these will need to be accessible from other sdk code and tests

4d9e0bf

add button to objc sample app to send new feedback in addition to the…

e1e8f9e

… old

also do it for iOS-Swift and differentiate the feedback

fd60904

add new docs link to headerdoc

7e13131

deprecate old methods and types

00b833a

more exposures and deprecations

ace8109

deprecation workarounds in swift test code

bd9301e

fix broken preexisting test

a31eac6

fixing new replacement test cases, plus a todo to fix the last one

623391d

Format code

105cde7

armcknight commented Feb 20, 2025

View reviewed changes

Tests/SentryTests/SentryClientTests.swift Outdated Show resolved Hide resolved

armcknight added 2 commits February 20, 2025 14:30

improve deprecation message and remove todo

b0a9f3f

found a way to validate feedback events are sent

5024f37

armcknight commented Feb 20, 2025

View reviewed changes

armcknight marked this pull request as ready for review February 20, 2025 23:33

philprime approved these changes Feb 21, 2025

View reviewed changes

philprime mentioned this pull request Feb 21, 2025

fix(feedback): last minute tweaks to apis to align implementation and docs #4873

Open

philipphofmann approved these changes Feb 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(feedback): initial release #4874

feat(feedback): initial release #4874

armcknight commented Feb 19, 2025 •

edited

Loading

github-actions bot commented Feb 19, 2025 •

edited

Loading

codecov bot commented Feb 19, 2025 •

edited

Loading

github-actions bot commented Feb 19, 2025 •

edited

Loading

Baseline results on branch: armcknight/feat(feedback)/final-fixes

Startup times

App size

Previous results on branch: armcknight/feat(feedback)/initial-release

Startup times

App size

armcknight Feb 20, 2025

philipphofmann Feb 21, 2025

philprime left a comment

philprime Feb 21, 2025

philprime Feb 21, 2025

philprime Feb 21, 2025

philipphofmann left a comment

philipphofmann Feb 21, 2025

armcknight Feb 21, 2025

philipphofmann Feb 21, 2025

philipphofmann Feb 21, 2025 •

edited by armcknight

Loading

philipphofmann Feb 21, 2025

philipphofmann Feb 21, 2025

philipphofmann Feb 21, 2025

		typedef void (^SentryUserFeedbackConfigurationBlock)(
		SentryUserFeedbackConfiguration *_Nonnull configuration);

		* @note This is unrelated to @c SentrySDK.captureUserFeedback which is the old method of submitting
		* user feedback you've already gathered via your own UI

		* (see https://docs.sentry.io/platforms/apple/user-feedback/#user-feedback-api). The new method
		* uses either this block to configure and a widget and UI form to gather feedback, or directly

feat(feedback): initial release #4874

Are you sure you want to change the base?

feat(feedback): initial release #4874

Conversation

armcknight commented Feb 19, 2025 • edited Loading

github-actions bot commented Feb 19, 2025 • edited Loading

codecov bot commented Feb 19, 2025 • edited Loading

Codecov Report

github-actions bot commented Feb 19, 2025 • edited Loading

Performance metrics 🚀

Baseline results on branch: armcknight/feat(feedback)/final-fixes

Startup times

App size

Previous results on branch: armcknight/feat(feedback)/initial-release

Startup times

App size

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philprime left a 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

philipphofmann left a 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

philipphofmann Feb 21, 2025 • edited by armcknight Loading

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

armcknight commented Feb 19, 2025 •

edited

Loading

github-actions bot commented Feb 19, 2025 •

edited

Loading

codecov bot commented Feb 19, 2025 •

edited

Loading

github-actions bot commented Feb 19, 2025 •

edited

Loading

philipphofmann Feb 21, 2025 •

edited by armcknight

Loading