feat: implement the new User Feedback API

[jpnurmi]

Sentry API: User Feedback vs. User Report - Deprecated

See also:

• Provide user-feedback capability to the Native SDK #885 (comment)

  While the original function for capturing user feedback (that requires event_id) was called CaptureUserFeedback, we decided to name the new one CaptureFeedback.

[github-actions]

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

Generated by 🚫 dangerJS against 80f1f61

[jpnurmi]

What if we don't change the sentry-native API at all but wire it up with the new Sentry Feedback API under the hood, and allow passing NULL as event ID?

[jpnurmi]

Option 1

Keep the old API as is. Add the new API alongside. (The way this PR started.)

// Old API
sentry_value_t sentry_value_new_user_feedback(const sentry_uuid_t *uuid,
                                              const char *name,
                                              const char *email,
                                              const char *comments);
void sentry_capture_user_feedback(sentry_value_t user_feedback);

// New API
sentry_value_t sentry_value_new_feedback(const char *message,
                                         const char *contact_email,
                                         const char *name,
                                         const sentry_uuid_t *associated_event_id);

void sentry_capture_feedback(sentry_value_t feedback);

Option 2

Wire up the existing API with the new Sentry Feedback API, and allow passing NULL for the event ID. Retain backward compatibility by checking feedback fields. (The way this PR is currently implemented.)

sentry_value_t old_feedback = sentry_value_new_object();
sentry_value_set_by_key(old_feedback, "event_id", ...);
/* ... */
sentry_capture_feedback(old_feedback);

// vs.

sentry_value_t new_feedback = sentry_value_new_user_feedback(NULL, name, email, comments);
sentry_capture_feedback(new_feedback);

Option 3

Something between options 1 and 2. For example, have one unified sentry_value_new_user_feedback but separate sentry_capture_user_feedback (old) and capture_feedback (new) to avoid unexpected behavioral changes for existing users.

sentry_value_t feedback = sentry_value_new_user_feedback(NULL, name, email, comments);

sentry_capture_user_feedback(feedback);  // Old API
// vs.
sentry_capture_feedback(feedback); // New API

[jpnurmi]

The docs claim that the old API should still work:

This is an older, deprecated way of submitting user feedback. It continues to work so older SDKs won't break. New implementations should follow the feedback item type described above.

However, with the current sentry-native master based on the old feedback endpoint, I'm unable to get any feedback to show up at all in a self-hosted Sentry version 25.6.2. With the snippet copied from the example, an event shows up but there's no sign of feedback:

    sentry_value_t event = sentry_value_new_message_event(
        SENTRY_LEVEL_INFO, "my-logger", "Hello user feedback!");
    sentry_uuid_t event_id = sentry_capture_event(event);

    sentry_value_t user_feedback = sentry_value_new_user_feedback(
        &event_id, "some-name", "some-email", "some-comment");

    sentry_capture_user_feedback(user_feedback);

Switching to the new endpoint helps.

[supervacuus]

basic design premise

What if we don't change the sentry-native API at all but wire it up with the new Sentry Feedback API under the hood, and allow passing NULL as event ID?

Tbh, that is how I expected this to be implemented. The users aren't interested in how this interacts with the endpoint API; they are only interested in whether they must connect this to an event or not. And the current interface can already make this quite explicit.

It is challenging to be confronted with a "new" API with an essentially identical interface but a slightly different name. Especially considering that the new name highlights something that isn't relevant if you are not concerned with how the backend handles this.

decision on interface options

So, I would opt for "Option 2". There are two aspects of the other two options that I do not understand (but might be irrelevant):

• "Option 1" shows separate feedback constructors, but they differ only by name and parameter order. Even the new API provides an event UUID parameter. If that is still optional in the new API, how can we distinguish between old and new when constructing feedback in "Option 2"? How would "Option 2" differ from switching the internal implementation entirely to the new replay-team user feedback?
• In "Option 3", you mention that separating the capture functions might "avoid unexpected behavioral changes for existing users." What kind of unexpected behavioral changes did you have in mind?

does the old API still work?

The docs claim that the old API should still work:

This is an older, deprecated way of submitting user feedback. It continues to work so older SDKs won't break. New implementations should follow the feedback item type described above.

However, with the current sentry-native master based on the old feedback endpoint, I'm unable to get any feedback to show up at all in a self-hosted Sentry version 25.6.2.

As is often the case, the last question asked should be at the start of the entire endeavor :-) Two things here:

• The "old" interface was thankfully implemented by @tustanivsky, and the primary driver and probably sole user is the user-feedback implementation in the Unreal SDK. He might be able to provide more information on whether this still works as expected in the Unreal SDK.
• I just tried to send feedback with the old mechanism, and it still seems to work (verbatim from the example using the master): https://sentry-sdks.sentry.io/issues/feedback/?feedbackSlug=sentry-native%3A6736275558&project=4506178389999616&referrer=feedback_list_page&statsPeriod=1h
• I hope you agree that we should clarify the current behavior (and its continued support) and why it doesn't work in your setup before we have the discussions regarding backwards compatibility or actual code review.

[jpnurmi]

Glad to hear you like option 2. 👍

I started with option 1 because other SDKs had gone that route. Things got quickly confusing, so I decided to try option 2 instead. The idea behind option 3 was just to let users opt in because the new and old endpoints behave differently.

Thanks for pointing out that the old endpoint still works as expected. Apparently, old-style user feedback can get quietly dropped in multiple ways with sentry-native. Errors about invalid emails or too old events didn't surface when sending envelopes with the sentry-native API. Trying to send the same feedback data straight to the feedback endpoint with curl cli revealed the problems. Confusingly enough, the new API does accept invalid emails and has no limitation on how old an event it can be associated with.

[supervacuus]

Apparently, old-style user feedback can get quietly dropped in multiple ways with sentry-native. Errors about invalid emails or too old events didn't surface when sending envelopes with the sentry-native API. Trying to send the same feedback data straight to the feedback endpoint with curl cli revealed the problems. Confusingly enough, the new API does accept invalid emails and has no limitation on how old an event it can be associated with.

I think all of what you describe hints even more at replacing the current implementation entirely with the new API. What is the reason to maintain this? What can't the new feedback API do that the old can (especially since we expose the same interfaces in the SDK, or at least the new is a strict superset of the old)?

[jpnurmi]

Ok, what about converting old feedback objects to the new format? This would help to avoid breaking sentry-unreal, which doesn't use sentry_value_new_user_feedback but constructs feedback objects by hand:

https://github.com/getsentry/sentry-unreal/blob/c9b9d320cbef96dbb7dbd3f1d2e0190e43cb126f/plugin-dev/Source/Sentry/Private/GenericPlatform/GenericPlatformSentryUserFeedback.cpp

[supervacuus]

Ok, what about converting old feedback objects to the new format? This would help to avoid breaking sentry-unreal, which doesn't use sentry_value_new_user_feedback but constructs feedback objects by hand

If the conversion can be done unambiguously, then this makes sense as a smooth upgrade path.

However, docs should mention that conversion takes place and stress the adoption of the new format (as the old is deprecated).

Especially with downstream SDKs, I view this as a more minor problem, as they typically control the version of the Native SDK at the distribution level (i.e., they are not confronted with arbitrary binary builds of the Native SDK in the wild). This means they can switch to the new format as soon as they have the next release and resources to implement the change.

[jpnurmi]

Some feedback from @bruno-garcia about repurposed capture_user_feedback vs. separate new capture_feedback:

We’d like to standardize on the new name though.
But more importantly, it’s a breaking change for self hosted sentry that don’t have support for the new envelope format
so I’d add the new function. keep the old one for a while. sentry converts it to the new format on the backend. eventually we drop it with a breaking change notice
meanwhile mark it as obsolete so folks know to migrate to the new one

With this in mind, I'm inclined to revert to option 1.

[supervacuus]

Some feedback from @bruno-garcia about repurposed capture_user_feedback vs. separate new capture_feedback:

We’d like to standardize on the new name though.
But more importantly, it’s a breaking change for self hosted sentry that don’t have support for the new envelope format
so I’d add the new function. keep the old one for a while. sentry converts it to the new format on the backend. eventually we drop it with a breaking change notice
meanwhile mark it as obsolete so folks know to migrate to the new one

With this in mind, I'm inclined to revert to option 1.

:-) Well, I have made my points, I hope the intersection between users that run on outdated self-hosted Sentry and rely on the current user-report interface is big enough to make this confusing for everyone else. Just to be clear:

When we say "standardize on the new name", then we mean which name? Because the header of the docs says "User Feedback", which in option 1 is the name of the old capture function, right?

Besides this, if the requirements were there, we could have spared the back and forth. Happy to go with whatever fits the goals.

[jpnurmi]

Sorry, my bad. I didn't have the full background but was focused on switching to the new endpoint to unblock getsentry/sentry-godot#204 and got confused with the duplicate API while updating tests... 😅

[supervacuus]

Another way to clean this up would be to introduce deliberate SDK breakage:

• rename the current implementation sentry_value_new_user_report()/sentry_capture_user_report() and immediately deprectate it. This allows everyone to explicitly use the old report envelope item type. This only breaks the SDK interface for those users who must use the old envelope item type and already have client code that depends on the current implementation.
• introduce sentry_value_new_feedback()/sentry_capture_feedback() or keep sentry_value_new_user_feedback()/sentry_capture_user_feedback() for the new envelope item type.
• If we keep the old name (== user_feedback), then users who already rely on it are upgraded to the new version without further changes. Of course, this depends on whether we adapt the parameters.
• If we introduce the new ("feedback" without "user"), then we break that path too, and existing users have to consciously choose between the old and the new.

This way we can:

• normalize on the correct naming
• support users with outdated self-hosting versions
• have a deliberate one-time breakage in the SDK API, which will hurt a small percentage of users, once, but clear delineation between the two until the old interface is removed.

I am not presenting this as yet another proposal, but I wanted to offer the option of SDK breakage, in case you're still considering alternatives.

[supervacuus]

Sorry, my bad. I didn't have the full background but was focused on switching to the new endpoint to unblock getsentry/sentry-godot#204 and got confused with the duplicate API while updating tests... 😅

No worries, it was a necessary exchange.

[jpnurmi]

Thanks for an interesting suggestion. A deliberate break is indeed quite tempting. According to GitHub's public code search, you might be right that sentry-unreal is probably the only user of this API.

https://github.com/search?q=sentry_capture_user_feedback+NOT+path%3A**%2Fsentry.h+NOT+path%3A**%2Fsentry_core.c+NOT+path%3A**%2Fexample.c&type=code&ref=advsearch

Nevertheless, since sentry_capture_feedback is wanted for consistency with other SDKs, adding sentry_capture_user_report on the side seems inconsistent. What if we just add the new API and mark the old API more visibly deprecated with #1308?

[supervacuus]

Nevertheless, since sentry_capture_feedback is wanted for consistency with other SDKs, adding sentry_capture_user_report on the side seems inconsistent. What if we just add the new API and mark the old API more visibly deprecated with #1308?

As I mentioned earlier, this wasn't intended as a new proposal. Given the relatively narrow requirements (which this should have started with), we should opt for option 1, even if it is far from ideal from the perspective of most users.

[jpnurmi]

GCC and Clang allow calling a deprecated function in the same compilation unit, but MSVC throws a warning

D:\a\sentry-native\sentry-native\src\sentry_value.c(1353,12): error C2220: the following warning is treated as an error [C:\Users\runneradmin\AppData\Local\Temp\pytest-of-runneradmin\pytest-0\cmake0\sentry.vcxproj]
D:\a\sentry-native\sentry-native\src\sentry_value.c(1353,12): warning C4996: 'sentry_value_new_user_feedback_n': Use `sentry_value_new_feedback_n` instead [C:\Users\runneradmin\AppData\Local\Temp\pytest-of-runneradmin\pytest-0\cmake0\sentry.vcxproj]

[supervacuus]

Looks great! Thanks for going the rounds with me @jpnurmi!