You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "Usiel (via GitHub)" <gi...@apache.org> on 2025/03/11 05:49:39 UTC

[PR] chore(reports): add task for slack channels warm-up [superset]

Usiel opened a new pull request, #32585:
URL: https://github.com/apache/superset/pull/32585

   ### SUMMARY
   
   Adds a celery task to warm up the Slack channel cache and allows admins to schedule the same task (with example). This is helpful for cases where retrieval of the full channel list takes minutes.
   
   #### Changes to the `get_channels` function
   
   The Slack conversations API is not necessarily faster when using the `types` filter: For example, when we filter for private channels, the cursor will still iterate over all public channels and the number of requests remains the same, only the data transmitted is less.
   
   With this in mind, I believe it is more efficient for us to cache only a single value for `get_channels()` and avoid having multiple cache values for each possible value of `types`. Doing the filtering in `get_channels_with_search` on demand doesn't add much cost.
   
   Additionally, the `limit` param is also not relevant for the cache key and is inlined.
   
   ... and, of course, this change makes the celery task more sensible :)
   
   #### Warning
   
   There is a minor API change in this PR: Previously, we followed Slack's [conversations.list](https://api.slack.com/methods/conversations.list) convention, where empty an `types` param means that only public channels are returned. After this PR we will return all channel types when `types` is empty or null. To me that feels more logical, but if people feel we should stick to previous approach I'm OK with changing this.
   
   ### TESTING INSTRUCTIONS
   
   Pre:
   - Set `SLACK_API_TOKEN`
   - Enable `ALERT_REPORTS`
   
   #### Test Celery Task
   
   ```
   celery --app=superset.tasks.celery_app:app call slack.cache_channels
   ```
   
   **Expected**: Task is launched and result is cached.
   
   #### Test Beat Schedule Example
   
   1. Uncomment the beat schedule entry for `slack.cache_channels`
   2. [Optional]: Adapt schedule to trigger every minute (or set time)
   
   **Expected**: Task is scheduled and result is cached.
   
   #### Test REST API
   
   Try for `TYPES={"public_channel", "private_channel", "", "public_channel, private_channel"}`
   
   ```
   curl -X 'GET' \
     'http://localhost:8088/api/v1/report/slack_channels/?q=(types%3A!(<TYPES>))' \
     -H 'accept: application/json' \
     -H 'Authorization: Bearer <TOKEN>'
   ```
   
   **Expected**: Cached value is used, filtered and returned.
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [x] Has associated issue: https://github.com/apache/superset/issues/32480
   - [ ] Required feature flags:
   - [ ] Changes UI
   - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
     - [ ] Migration is atomic, supports rollback & is backwards-compatible
     - [ ] Confirm DB migration upgrade and downgrade tested
     - [ ] Runtime estimates and downtime expectations provided
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   
   cc @Vitor-Avila since this builds on your recent work in https://github.com/apache/superset/pull/32529
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] chore(reports): add task for slack channels warm-up [superset]

Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #32585:
URL: https://github.com/apache/superset/pull/32585#discussion_r1989807373


##########
superset/utils/slack.py:
##########
@@ -71,11 +71,12 @@ def get_channels(limit: int, extra_params: dict[str, Any]) -> list[SlackChannelS
     client = get_slack_client()
     channel_schema = SlackChannelSchema()
     channels: list[SlackChannelSchema] = []
+    extra_params = {"types": ",".join(SlackChannelTypes)}
     cursor = None
 
     while True:
         response = client.conversations_list(
-            limit=limit, cursor=cursor, exclude_archived=True, **extra_params
+            limit=999, cursor=cursor, exclude_archived=True, **extra_params

Review Comment:
   why are we hardcoding this now?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] chore(reports): add task for slack channels warm-up [superset]

Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #32585:
URL: https://github.com/apache/superset/pull/32585#discussion_r1989187801


##########
superset/config.py:
##########
@@ -1046,6 +1047,11 @@ class CeleryConfig:  # pylint: disable=too-few-public-methods
         #     "schedule": crontab(minute="*", hour="*"),
         #     "kwargs": {"retention_period_days": 180},
         # },
+        # Uncomment to enable Slack channel cache warm-up
+        # "slack.cache_channels": {
+        #     "task": "slack.cache_channels",
+        #     "schedule": crontab(minute="0", hour="*"),
+        # },

Review Comment:
   Nice!



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] chore(reports): add task for slack channels warm-up [superset]

Posted by "Usiel (via GitHub)" <gi...@apache.org>.
Usiel commented on code in PR #32585:
URL: https://github.com/apache/superset/pull/32585#discussion_r1988459838


##########
superset/tasks/slack.py:
##########
@@ -0,0 +1,27 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import logging
+
+from superset.extensions import celery_app
+from superset.utils.slack import get_channels
+
+logger = logging.getLogger(__name__)
+
+
+@celery_app.task(name="slack.cache_channels")
+def cache_channels() -> None:
+    get_channels(force=True)

Review Comment:
   The Slack client handles both connection and (because we added it) rate-limit errors.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] chore(reports): add task for slack channels warm-up [superset]

Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #32585:
URL: https://github.com/apache/superset/pull/32585#discussion_r1989195271


##########
superset/tasks/slack.py:
##########
@@ -0,0 +1,27 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import logging
+
+from superset.extensions import celery_app
+from superset.utils.slack import get_channels
+
+logger = logging.getLogger(__name__)
+
+
+@celery_app.task(name="slack.cache_channels")
+def cache_channels() -> None:
+    get_channels(force=True)

Review Comment:
   Could we enforce calling it with a `cache_timeout` as well? Just to prevent creating cache items that don't have an expiration (since it's a scheduled task it would always refresh on schedule, but there's an edge case of a user disabling the task and then it would be there until someone refreshes through the UI which has a `cache_timeout`).
   
   I was actually wondering if we should have a new config like `SLACK_CACHE_TIMEOUT` (since this is hardcoded in the code, keeping it in `config.py` might make more sense). This was supposed to be part of my PR, but just thought about it after having it merged so was planning to do a follow up in the future. If we have a config, we could use it here + in the API config.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] chore(reports): add task for slack channels warm-up [superset]

Posted by "Usiel (via GitHub)" <gi...@apache.org>.
Usiel commented on code in PR #32585:
URL: https://github.com/apache/superset/pull/32585#discussion_r1990498594


##########
superset/utils/slack.py:
##########
@@ -71,11 +71,12 @@ def get_channels(limit: int, extra_params: dict[str, Any]) -> list[SlackChannelS
     client = get_slack_client()
     channel_schema = SlackChannelSchema()
     channels: list[SlackChannelSchema] = []
+    extra_params = {"types": ",".join(SlackChannelTypes)}
     cursor = None
 
     while True:
         response = client.conversations_list(
-            limit=limit, cursor=cursor, exclude_archived=True, **extra_params
+            limit=999, cursor=cursor, exclude_archived=True, **extra_params

Review Comment:
   I inlined the `limit` because it never governed the amount of channels returned by the function. The function always returns all channels. The `limit` parameter here only governs the amount of channels returned by each Slack API response. 
   
   Going for the maximum (999) allowed by Slack is most efficient, imo. Lower values are more likely to result in rate-limiting (which is based on requests, not rows returned or similar).



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] chore(reports): add task for slack channels warm-up [superset]

Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #32585:
URL: https://github.com/apache/superset/pull/32585#issuecomment-2712748010

   ## [Codecov](https://app.codecov.io/gh/apache/superset/pull/32585?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   Attention: Patch coverage is `58.82353%` with `7 lines` in your changes missing coverage. Please review.
   > Project coverage is 65.76%. Comparing base [(`76d897e`)](https://app.codecov.io/gh/apache/superset/commit/76d897eaa2f9e137102bc194c2e3109c29d0348f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`fc029f3`)](https://app.codecov.io/gh/apache/superset/commit/fc029f3e4437456a7bc0bf4ee300d7a0623f4cad?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   > Report is 1559 commits behind head on master.
   
   | [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/32585?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
   |---|---|---|
   | [superset/tasks/slack.py](https://app.codecov.io/gh/apache/superset/pull/32585?src=pr&el=tree&filepath=superset%2Ftasks%2Fslack.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdGFza3Mvc2xhY2sucHk=) | 0.00% | [7 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/32585?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   
   <details><summary>Additional details and impacted files</summary>
   
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #32585      +/-   ##
   ==========================================
   + Coverage   60.48%   65.76%   +5.28%     
   ==========================================
     Files        1931      549    -1382     
     Lines       76236    39371   -36865     
     Branches     8568        0    -8568     
   ==========================================
   - Hits        46114    25894   -20220     
   + Misses      28017    13477   -14540     
   + Partials     2105        0    -2105     
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/superset/pull/32585/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [hive](https://app.codecov.io/gh/apache/superset/pull/32585/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `48.45% <11.76%> (-0.71%)` | :arrow_down: |
   | [javascript](https://app.codecov.io/gh/apache/superset/pull/32585/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [presto](https://app.codecov.io/gh/apache/superset/pull/32585/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `52.95% <11.76%> (-0.86%)` | :arrow_down: |
   | [python](https://app.codecov.io/gh/apache/superset/pull/32585/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `65.76% <58.82%> (+2.28%)` | :arrow_up: |
   | [unit](https://app.codecov.io/gh/apache/superset/pull/32585/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `61.36% <58.82%> (+3.74%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   </details>
   
   [:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/32585?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).   
   :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   
   <details><summary>🚀 New features to boost your workflow: </summary>
   
   - ❄ [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
   - 📦 [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
   </details>


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] chore(reports): add task for slack channels warm-up [superset]

Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #32585:
URL: https://github.com/apache/superset/pull/32585#discussion_r1988460197


##########
superset/tasks/slack.py:
##########
@@ -0,0 +1,27 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import logging
+
+from superset.extensions import celery_app
+from superset.utils.slack import get_channels
+
+logger = logging.getLogger(__name__)
+
+
+@celery_app.task(name="slack.cache_channels")
+def cache_channels() -> None:
+    get_channels(force=True)

Review Comment:
   Thank you for pointing that out, Usiel. It's good to know that the Slack client handles connection and rate-limit errors. However, it might still be beneficial to have our own retry mechanism to provide a standard way of handling unexpected failures and improve resilience. While the exceptions thrown by the client library are managed, any others that may occur in the future won't be. It can also provide a uniform way to manage logging for these errors. However, these are just suggestions for future improvements and not a requirement for this pull request.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] chore(reports): add task for slack channels warm-up [superset]

Posted by "Usiel (via GitHub)" <gi...@apache.org>.
Usiel commented on code in PR #32585:
URL: https://github.com/apache/superset/pull/32585#discussion_r1990502460


##########
superset/tasks/slack.py:
##########
@@ -0,0 +1,27 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import logging
+
+from superset.extensions import celery_app
+from superset.utils.slack import get_channels
+
+logger = logging.getLogger(__name__)
+
+
+@celery_app.task(name="slack.cache_channels")
+def cache_channels() -> None:
+    get_channels(force=True)

Review Comment:
   Makes sense! Done and done.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] chore(reports): add task for slack channels warm-up [superset]

Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #32585:
URL: https://github.com/apache/superset/pull/32585#discussion_r1988451377


##########
superset/utils/slack.py:
##########
@@ -99,18 +99,25 @@ def get_channels_with_search(
     all channels and filter them ourselves
     This will search by slack name or id
     """
-    extra_params = {}
-    extra_params["types"] = ",".join(types) if types else None
     try:
         channels = get_channels(
-            limit=limit,
-            extra_params=extra_params,
             force=force,
             cache_timeout=86400,
         )

Review Comment:
   ### Function Call Parameter Mismatch <sub>![category Functionality](https://img.shields.io/badge/Functionality-0284c7)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   The get_channels() function call in get_channels_with_search() includes parameters 'force' and 'cache_timeout' that are not defined in the function signature.
   
   ###### Why this matters
   This will raise a TypeError at runtime since the function doesn't accept these parameters, breaking the channel retrieval functionality.
   
   ###### Suggested change ∙ *Feature Preview*
   Remove the unsupported parameters from the get_channels() call:
   ```python
   channels = get_channels()
   ```
   
   
   </details>
   
   <sub>
   
   [![Report a problem with this comment](https://img.shields.io/badge/Report%20a%20problem%20with%20this%20comment-gray.svg?logo=data:image/svg+xml;base64,PHN2ZyB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciIHdpZHRoPSIyNCIgaGVpZ2h0PSIyNCIgdmlld0JveD0iMCAwIDI0IDI0IiBmaWxsPSJub25lIiBzdHJva2U9IiNmNWVjMDAiIHN0cm9rZS13aWR0aD0iMiIgc3Ryb2tlLWxpbmVjYXA9InJvdW5kIiBzdHJva2UtbGluZWpvaW49InJvdW5kIiBjbGFzcz0ibHVjaWRlIGx1Y2lkZS10cmlhbmdsZS1hbGVydCI+PHBhdGggZD0ibTIxLjczIDE4LTgtMTRhMiAyIDAgMCAwLTMuNDggMGwtOCAxNEEyIDIgMCAwIDAgNCAyMWgxNmEyIDIgMCAwIDAgMS43My0zIi8+PHBhdGggZD0iTTEyIDl2NCIvPjxwYXRoIGQ9Ik0xMiAxN2guMDEiLz48L3N2Zz4=)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/625dcf51-4662-4260-a228-ceb29ed99779?suggestedFixEnabled=true)
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:0dfae944-4903-479a-8a4c-94f15350aa51 -->
   



##########
superset/tasks/slack.py:
##########
@@ -0,0 +1,27 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import logging
+
+from superset.extensions import celery_app
+from superset.utils.slack import get_channels
+
+logger = logging.getLogger(__name__)
+
+
+@celery_app.task(name="slack.cache_channels")
+def cache_channels() -> None:
+    get_channels(force=True)

Review Comment:
   ### Missing Celery Task Retry Mechanism <sub>![category Performance](https://img.shields.io/badge/Performance-4f46e5)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   The Celery task lacks retry mechanisms and error handling for network failures when caching Slack channels.
   
   ###### Why this matters
   Network operations can fail transiently. Without retries, a temporary network issue could cause the cache to remain empty, leading to additional API calls and performance degradation.
   
   ###### Suggested change ∙ *Feature Preview*
   Add retry mechanism and error handling to the Celery task:
   ```python
   @celery_app.task(name="slack.cache_channels", bind=True, max_retries=3)
   def cache_channels(self) -> None:
       try:
           get_channels(force=True)
       except Exception as ex:
           self.retry(countdown=exponential_backoff(self.request.retries), exc=ex)
   ```
   
   
   </details>
   
   <sub>
   
   [![Report a problem with this comment](https://img.shields.io/badge/Report%20a%20problem%20with%20this%20comment-gray.svg?logo=data:image/svg+xml;base64,PHN2ZyB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciIHdpZHRoPSIyNCIgaGVpZ2h0PSIyNCIgdmlld0JveD0iMCAwIDI0IDI0IiBmaWxsPSJub25lIiBzdHJva2U9IiNmNWVjMDAiIHN0cm9rZS13aWR0aD0iMiIgc3Ryb2tlLWxpbmVjYXA9InJvdW5kIiBzdHJva2UtbGluZWpvaW49InJvdW5kIiBjbGFzcz0ibHVjaWRlIGx1Y2lkZS10cmlhbmdsZS1hbGVydCI+PHBhdGggZD0ibTIxLjczIDE4LTgtMTRhMiAyIDAgMCAwLTMuNDggMGwtOCAxNEEyIDIgMCAwIDAgNCAyMWgxNmEyIDIgMCAwIDAgMS43My0zIi8+PHBhdGggZD0iTTEyIDl2NCIvPjxwYXRoIGQ9Ik0xMiAxN2guMDEiLz48L3N2Zz4=)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f51de996-d591-4dc9-9118-5fec8d9ed52c?suggestedFixEnabled=true)
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:d730bf75-7580-4e4d-ade9-b40932d8178c -->
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] chore(reports): add task for slack channels warm-up [superset]

Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #32585:
URL: https://github.com/apache/superset/pull/32585#discussion_r1988460919


##########
superset/utils/slack.py:
##########
@@ -99,18 +99,25 @@ def get_channels_with_search(
     all channels and filter them ourselves
     This will search by slack name or id
     """
-    extra_params = {}
-    extra_params["types"] = ",".join(types) if types else None
     try:
         channels = get_channels(
-            limit=limit,
-            extra_params=extra_params,
             force=force,
             cache_timeout=86400,
         )

Review Comment:
   Thank you for clarifying, @Usiel. I now understand that the `force` and `cache_timeout` parameters are provided by the `utils.cache.memoized_func` decorator. I appreciate your patience and explanation.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] chore(reports): add task for slack channels warm-up [superset]

Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #32585:
URL: https://github.com/apache/superset/pull/32585#discussion_r1996451233


##########
superset/tasks/slack.py:
##########
@@ -0,0 +1,35 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import logging
+
+from flask import current_app
+
+from superset.extensions import celery_app
+from superset.utils.slack import get_channels
+
+logger = logging.getLogger(__name__)
+
+
+@celery_app.task(name="slack.cache_channels")
+def cache_channels() -> None:
+    try:
+        get_channels(
+            force=True, cache_timeout=current_app.config["SLACK_CACHE_TIMEOUT"]

Review Comment:
   @dpgaspar can you confirm if this task would work properly for fetching the slack token (when it is a callable) for multi-tenant applications? Do we need to pass in some user context to this function? 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] chore(reports): add task for slack channels warm-up [superset]

Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #32585:
URL: https://github.com/apache/superset/pull/32585#discussion_r1989205771


##########
superset/tasks/slack.py:
##########
@@ -0,0 +1,27 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import logging
+
+from superset.extensions import celery_app
+from superset.utils.slack import get_channels
+
+logger = logging.getLogger(__name__)
+
+
+@celery_app.task(name="slack.cache_channels")
+def cache_channels() -> None:
+    get_channels(force=True)

Review Comment:
   Also, since this is calling `get_channels()` directly, could we do it in a `try/except` block and log exceptions? Raising an error wouldn't make much sense as this is async, but I think logging is always a good thing.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] chore(reports): add task for slack channels warm-up [superset]

Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on PR #32585:
URL: https://github.com/apache/superset/pull/32585#issuecomment-2714130883

   This is super cool! In regards to:
   
   > There is a minor API change in this PR: Previously, we followed Slack's [conversations.list](https://api.slack.com/methods/conversations.list) convention, where empty an types param means that only public channels are returned. After this PR we will return all channel types when types is empty or null. To me that feels more logical, but if people feel we should stick to previous approach I'm OK with changing this.
   
   That makes sense to me (if you're not including a filter, you're expecting to get all data), but I'm not super familiar with all rules for API changes, so I'd like to get @eschutho's thoughts here. Would this be considered a breaking change? While it's an API change, this API endpoint is mostly used by the bot (I can't think of a reason someone would interact with this API directly).
   
   Regardless, I believe that doesn't hold us from merging, but curious if there should be mention of this in the `UPDATING.md` file and also if we can include it in the next OSS release version.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] chore(reports): add task for slack channels warm-up [superset]

Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #32585:
URL: https://github.com/apache/superset/pull/32585#discussion_r1992498266


##########
superset/config.py:
##########
@@ -1488,6 +1494,7 @@ def EMAIL_HEADER_MUTATOR(  # pylint: disable=invalid-name,unused-argument  # noq
 # Slack API token for the superset reports, either string or callable
 SLACK_API_TOKEN: Callable[[], str] | str | None = None
 SLACK_PROXY = None
+SLACK_CACHE_TIMEOUT = int(timedelta(days=1).total_seconds())

Review Comment:
   thanks!



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] chore(reports): add task for slack channels warm-up [superset]

Posted by "Usiel (via GitHub)" <gi...@apache.org>.
Usiel commented on code in PR #32585:
URL: https://github.com/apache/superset/pull/32585#discussion_r1990501785


##########
superset/utils/slack.py:
##########
@@ -71,11 +71,12 @@ def get_channels(limit: int, extra_params: dict[str, Any]) -> list[SlackChannelS
     client = get_slack_client()
     channel_schema = SlackChannelSchema()
     channels: list[SlackChannelSchema] = []
+    extra_params = {"types": ",".join(SlackChannelTypes)}
     cursor = None
 
     while True:
         response = client.conversations_list(
-            limit=limit, cursor=cursor, exclude_archived=True, **extra_params
+            limit=999, cursor=cursor, exclude_archived=True, **extra_params

Review Comment:
   If we see a need we could add paging to our `/slack_channels` endpoint (and `get_channels_with_search(...)`). I don't know about that though, our only usage is to get all channels to give the user a choice for the report notification setting.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] chore(reports): add task for slack channels warm-up [superset]

Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #32585:
URL: https://github.com/apache/superset/pull/32585#issuecomment-2744386856

   Looks good to merge?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] chore(reports): add task for slack channels warm-up [superset]

Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on PR #32585:
URL: https://github.com/apache/superset/pull/32585#issuecomment-2744490445

   @rusackas my only question here is relating to the API change. While this API endpoint is solely used by the Slack integration (I can't think why a user would create any workflow on top of a Superset API endpoint that just lists Slack channels) I wonder if we need to mention this in `UPDATING.md` and if it has to be considered a breaking change or not.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] chore(reports): add task for slack channels warm-up [superset]

Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina merged PR #32585:
URL: https://github.com/apache/superset/pull/32585


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] chore(reports): add task for slack channels warm-up [superset]

Posted by "dpgaspar (via GitHub)" <gi...@apache.org>.
dpgaspar commented on code in PR #32585:
URL: https://github.com/apache/superset/pull/32585#discussion_r1998517906


##########
superset/tasks/slack.py:
##########
@@ -0,0 +1,35 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import logging
+
+from flask import current_app
+
+from superset.extensions import celery_app
+from superset.utils.slack import get_channels
+
+logger = logging.getLogger(__name__)
+
+
+@celery_app.task(name="slack.cache_channels")
+def cache_channels() -> None:
+    try:
+        get_channels(
+            force=True, cache_timeout=current_app.config["SLACK_CACHE_TIMEOUT"]

Review Comment:
   it should just work



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] chore(reports): add task for slack channels warm-up [superset]

Posted by "Usiel (via GitHub)" <gi...@apache.org>.
Usiel commented on code in PR #32585:
URL: https://github.com/apache/superset/pull/32585#discussion_r1988448497


##########
superset/utils/slack.py:
##########
@@ -99,18 +99,25 @@ def get_channels_with_search(
     all channels and filter them ourselves
     This will search by slack name or id
     """
-    extra_params = {}
-    extra_params["types"] = ",".join(types) if types else None
     try:
         channels = get_channels(
-            limit=limit,
-            extra_params=extra_params,
             force=force,
             cache_timeout=86400,
         )
     except (SlackClientError, SlackApiError) as ex:
         raise SupersetException(f"Failed to list channels: {ex}") from ex
 
+    if types and not len(types) == len(SlackChannelTypes):
+        conditions: list[Callable[[SlackChannelSchema], bool]] = []

Review Comment:
   The intention here was to make this more easily pluggable in the future if we add more types.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] chore(reports): add task for slack channels warm-up [superset]

Posted by "Usiel (via GitHub)" <gi...@apache.org>.
Usiel commented on code in PR #32585:
URL: https://github.com/apache/superset/pull/32585#discussion_r1988460633


##########
superset/utils/slack.py:
##########
@@ -99,18 +99,25 @@ def get_channels_with_search(
     all channels and filter them ourselves
     This will search by slack name or id
     """
-    extra_params = {}
-    extra_params["types"] = ",".join(types) if types else None
     try:
         channels = get_channels(
-            limit=limit,
-            extra_params=extra_params,
             force=force,
             cache_timeout=86400,
         )

Review Comment:
   The parameters are from utils.cache.memoized_func



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] chore(reports): add task for slack channels warm-up [superset]

Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #32585:
URL: https://github.com/apache/superset/pull/32585#discussion_r1989206231


##########
superset/utils/slack.py:
##########
@@ -99,18 +99,25 @@ def get_channels_with_search(
     all channels and filter them ourselves
     This will search by slack name or id
     """
-    extra_params = {}
-    extra_params["types"] = ",".join(types) if types else None
     try:
         channels = get_channels(
-            limit=limit,
-            extra_params=extra_params,
             force=force,
             cache_timeout=86400,
         )
     except (SlackClientError, SlackApiError) as ex:
         raise SupersetException(f"Failed to list channels: {ex}") from ex
 
+    if types and not len(types) == len(SlackChannelTypes):
+        conditions: list[Callable[[SlackChannelSchema], bool]] = []

Review Comment:
   Nice!



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org