-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: enable user feedback feature #3193
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3193 +/- ##
==========================================
- Coverage 99.01% 98.02% -0.99%
==========================================
Files 3 3
Lines 203 203
==========================================
- Hits 201 199 -2
- Misses 2 4 +2 ☔ View full report in Codecov by Sentry. |
Thanks @aldy505 ! |
Thanks @aldy505! This relates to a PR @hubertdeng123 and I were working on a while back: #3060. We had some problems getting relay to accept the envelopes when manually testing, something CORS related. You'll also need the option in #3061, though we can do that in a separate PR |
@aliu39 How did you guys encounter the CORS issue?
I don't quite get it why it's not |
It's been a while so I don't remember the details.. cc @hubertdeng123. I can look into it again later this week It is 1.0 for our SaaS site, but is 0 for self-hosted since we SH doesn't have the topic which your PR adds. |
Testing this locally, but encountering this error: It appears that hitting http://127.0.0.1:9000/organizations/sentry/feedback/?feedbackSlug=${project_slug}:${feedback_event_id} works. However, http://127.0.0.1:9000/organizations/sentry/feedback/ does not. |
It's an old known bug. Do you want to wait for the fix for it before we add the feature into SH? |
We still need to some time to debug the new topic, but it's not needed to enable this feature. To add this feature, technically all we need are the feature flags (can keep the option at 0.0). It'd definitely be nice to fix the frontend bug. We're making progress! I think you can merge this PR independent of that, though. |
Are you sure you're specifying the right flag? There's no such flag as that on features/temporary.py and features/permanent.py. The feature is working as intended on my side, testing it on my development environment (an actual server) using JS SDK v8.
The topic should be created automatically though, see #3121 |
Yeah the topic is created but we were having some problems ingesting thru it. The flags are right, they are programmatically generated by a feature handler |
@aldy505 Did you encounter the bug that I was facing when testing the feature? I'm wondering if all users running user feedback are experiencing the issue or just me. If all users are facing the issue, then I'd like to avoid merging for now. |
@hubertdeng123 Yeah I'm encountering it. But since I kind of want the feature and ingestion is not a problem, so I think it's still acceptable. |
From my tests, using the embed (triggered by |
An update: This PR didn't work. Seems like we need to postpone this one a bit.😢 @TheDevMinerTV said on Discord, that using What works so far:
I can't check whether the events are correctly ingested to Postgres/Clickhouse since I don't know how to check that. The feature flags
Removing both What me and @TheDevMinerTV tried:
@aliu39 @hubertdeng123 Let me know if you guys want to create a debug session with us for this. (Although I know every one of us live on a very different timezones) |
Thanks so much for debugging in-depth @aldy505 @TheDevMinerTV. Good call on removing the spam flags! There are extra settings for that feature not supported in SH yet. Hubert and I are in US Pacific time (PST). I'll try the tests you mentioned and see if I reproduce the same result. |
I'm going to prioritize something else, until we can finally have the time to debug all this. |
Since getsentry/sentry@ee95b68 is included in 24.7.1, I'm gonna debug everything this weekend. Go to #self-hosted channel on Discord to see me (potentially) spamming everything about User Feedback. |
Debugging update (not really an update since this is not a progress at all): So everything is correctly working during:
This is the Kafka payload on I have no idea on how this
So on Clickhouse, that project_id + event_id combination only exists on the And... now I'm stuck. |
Hi @aldy505 , I'm looking into this now. The issue platform system isn't owned by us so my knowledge goes about as far as you, between ingest-occurrences and Clickhouse. I can try asking some people from that team though. Just curious, why are you looking at the |
The correct table is Btw, I sent you a friend request on discord, feel free to reach out there if it's more convenient! |
Also this didn't solve the issue for me, but remember to add these flags, the first is especially important. Sorry I missed this for the develop docs |
Ok so weird thing is.. the data showed up |
Okay so it finally works on my instance. Can you guys also try it out? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working on my end too!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to work fine for me as well (can't approve because of GitHub permissions), just the frontend is crashing sometimes, but that doesn't belong here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code looks fine to me, although I have not tested this code locally
@TheDevMinerTV what kind of crashes are you experiencing? Is it related to getsentry/sentry#70136 at all? (We think this issue has been closed by a recent PR) |
Ref getsentry/sentry#66100 The topic and consumer has been deployed to all regions, and this option is 1.0 everywhere: https://github.com/search?q=repo%3Agetsentry%2Fsentry-options-automator%20topic.rollout&type=code. After getsentry/self-hosted#3193 was merged, this is true for self-hosted as well. We don't have to worry about a migration there because this is the first PR enabling user feedback in SH. Follow-up: remove from - getsentry - self-hosted - finally, options-automator
Closes #2646
This is based on https://develop.sentry.dev/feedback-architecture/ and some other stuff that I've used since User Feedback is in beta.
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.