-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat(integration): Add new endpoint to list channels #101321
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(integration): Add new endpoint to list channels #101321
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #101321 +/- ##
===========================================
+ Coverage 80.35% 81.04% +0.69%
===========================================
Files 8700 8701 +1
Lines 385849 385961 +112
Branches 24402 24402
===========================================
+ Hits 310038 312795 +2757
+ Misses 75460 72815 -2645
Partials 351 351 |
leeandher
left a comment
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 and tests look good, but I have a few callouts (detailed in the individual comments)
- Slack being paginated, makes it hard to do a typeahead dropdown, do we have a strategy around this?
- I would remove the
limitoptions on the API and just use the max if possible. IMO even if the payload is huge, the expensive DB query to list thousands of discord channels is done by discord 🤷 then we can use a static list dropdown on our frontend with every option available (for Discord and MSTeams)
I was able to test that this works for Discord btw! Unfortunately our test environment for MS Teams is not working at the moment so I couldn't test that :(
| limit_raw = request.GET.get("limit") | ||
| try: | ||
| limit = int(limit_raw) if limit_raw is not None else 50 | ||
| except ValueError: | ||
| limit = 50 | ||
| limit = max(1, min(limit, 200)) | ||
|
|
||
| cursor = request.GET.get("cursor") |
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.
So my first thought is that I don't think we need to be limiting this endpoint at all because it's not any more backend intensive on our end if we remove the limits, since the queries are all on the providers side. I guess the payload can get large, but I think the frontend part would be made easier if we could be confident that (for example) all the discord channels were in one payload.
We'd be able to use a static select dropdown instead of having to have a typeahead that makes a debounced query every time, but let me know if I'm misunderstanding.
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.
Yes, I can remove it. I originally added this “simulation” to be consistent with Slack, which offers pagination, but given the search + pagination issue, it no longer makes sense. I’ll update the code.
| return results, next_cursor | ||
|
|
||
|
|
||
| @region_silo_endpoint |
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.
I think since we're only calling integration APIs here, we can switch this to control for a latency improvement. All outbound requests to providers leave from the control silo so what will actually happen is
Browser -> Region
Region -> Control
Control -> Slack
Control <- Slack
Region <- Control
Browser <- Region
We can do Browser -> Control directly to save some steps
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.
good catch! thank you
| resp = client.conversations_list(**params).data | ||
| resp_data: dict[str, Any] = resp if isinstance(resp, dict) else {} | ||
| channels = resp_data.get("channels", []) or [] |
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.
So one of the reasons this doesn't already exist is because of the paginated APIs, at least for slack. There are some customer workspaces with >1000 channels, so they might not appear in the response even with the max limit set.
They also don't have a text filter on the endpoint, we couldn't filter the channels to the first 1000 matching discuss- for example, which prevented us from doing dropdowns or typeaheads on alert pages.
Do we have a way around this now? It's doesn't seem to be a problem for Discord and MS Teams if they give us every channel in one big response, but since Slack's API is paginated we didn't really have a good alternative.
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.
Actually as some background context, the way that we convert the channel name to a usable ID is here -- it's by using a specific API which (for some reason) Slack allows to be called without an ID, but the response will contain the ID we can save on an alert.
Maybe that could be useful here if we cant figure out the pagination problem above?
…egration-channels
Hey, thanks for the great feedback! I initially considered manually paginating Discord and MS Teams to be consistent with Slack, which already supports pagination - it’s nice because responses come back faster, but you are right about the search + pagination issue. I just checked and Slack does have an enterprise-only feature (see for example the admin.conversations.search) that could help, but we can’t rely on it since its for enterprise customers only and not available to all. Good point! |
|
🚨 Warning: This pull request contains Frontend and Backend changes! It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently. Have questions? Please ask in the |
leeandher
left a comment
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.
Looks good! Just a heads up though, there are some design pattern changes the team has been talking about that involve avoiding manually calling the integration API clients.
It's done with a few steps:
- Set up the method on the installation class (example)
- Get the installation from the
Integrationobject (example) - Call the new method (example)
It's our attempt to standardize how we hit these APIs but it's not a wide spread pattern and we haven't migrated all integrations to follow this yet. Approving to unblock, but if possible it'd be great if you could revisit this feature later to add the pattern above, though it's not urgent. Thanks again for looping us in on this feature!
hey, thanks for the heads up and examples! I’ll merge and take care of the migration in a follow-up PR 😉 |
#101508) **Problem** The Slack API has some limitations (see the [PR](#101321 (review)) discussion). We list up to 1,000 channels in the dropdown, but users might not find the one they want. To address this, we will allow users to manually enter a channel. If the channel was typed instead of selected from the list, we want to validate it on the component’s onBlur event. **Solution** We’re introducing a separate endpoint specifically for channel validation, since the form might not be complete and we cannot fire the same request used when submitting the form. This validation may not be necessary for MS Teams or Discord, but we can include it for consistency. Contributes to https://linear.app/getsentry/issue/TET-1229/implement-dropdown-or-validation-for-slack-field
**Problem** When creating a new project, users can configure alerts to be sent to a Slack, Discord, or Microsoft Teams channel. Currently, the channel name is entered manually in a text field, with no pre-validation to verify that the channel actually exists. As a result, users may enter an invalid or non-existent channel. The issue is only detected after the form is submitted, when Sentry attempts to validate the channel. If the channel is invalid, an error is returned and the project creation fails. The experience is further complicated when projects are created via the modal (for example, when a user selects a vanilla platform like JavaScript, a modal is displayed asking if they want to use a more specific framework, and they can submit the form from there). If project creation fails due to an invalid channel, the resulting error appears beneath the modal and is difficult to see. (This UI issue will be addressed in a separate PR.) **Solution** To improve the user experience, we propose replacing the manual text field with a searchable Select field where users can choose a channel from a list (This will be done in multiple PRs and this is the first one). This PR introduces a new endpoint: `/integrations/(slack/discord/msteams)/channels/` - For Slack, the API supports cursor-based pagination and limit parameter - For Microsoft Teams and Discord, which return the full channel list without pagination, we implement cursor and limit manually and cache results to reduce repeated API requests. This helps users select an existing channel and reduces the likelihood of project creation failing due to invalid channel. contributes to https://linear.app/getsentry/issue/TET-1229/implement-dropdown-or-validation-for-slack-field
Problem
When creating a new project, users can configure alerts to be sent to a Slack, Discord, or Microsoft Teams channel. Currently, the channel name is entered manually in a text field, with no pre-validation to verify that the channel actually exists.
As a result, users may enter an invalid or non-existent channel. The issue is only detected after the form is submitted, when Sentry attempts to validate the channel. If the channel is invalid, an error is returned and the project creation fails.
The experience is further complicated when projects are created via the modal (for example, when a user selects a vanilla platform like JavaScript, a modal is displayed asking if they want to use a more specific framework, and they can submit the form from there). If project creation fails due to an invalid channel, the resulting error appears beneath the modal and is difficult to see. (This UI issue will be addressed in a separate PR.)
Solution
To improve the user experience, we propose replacing the manual text field with a searchable Select field where users can choose a channel from a list (This will be done in multiple PRs and this is the first one).
This PR introduces a new endpoint:
/integrations/(slack/discord/msteams)/channels/This helps users select an existing channel and reduces the likelihood of project creation failing due to invalid channel.
contributes to https://linear.app/getsentry/issue/TET-1229/implement-dropdown-or-validation-for-slack-field