Skip to content
This repository has been archived by the owner on Sep 20, 2024. It is now read-only.

Slack: user groups speedup #4318

Merged
merged 4 commits into from
Jan 16, 2023
Merged

Conversation

kalisp
Copy link
Member

@kalisp kalisp commented Jan 16, 2023

Brief description

Query user and group information only if necessary.

Description

When message contains '@' character, translation between users and Slack user id is necessar. This operation might hit rate limit and slows down publish process.
Trigger this querying only if absolutely necessary.

Testing notes:

  1. publish something that should trigger Slack notification
  2. compare messages with and without @ character

Only trigger when message contains @ sign.
@kalisp kalisp added the type: bug Something isn't working label Jan 16, 2023
@kalisp kalisp requested a review from 64qam January 16, 2023 10:35
@kalisp kalisp self-assigned this Jan 16, 2023
Handles cases like @test\n@john.doe
Comment on lines 65 to 66
if users is None:
users, groups = client.get_users_and_groups()
Copy link
Collaborator

@BigRoy BigRoy Jan 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the speed of client.get_users_and_groups()? Is it very slow?

Since this is an InstancePlugin would it be maybe worth caching the result in context.data on the first run and any consecutive instances would re-use the value.

For e.g. 10 instances that'd bring it back from 10 calls to a single call.

Kind of like this:

def process(self, instance):
# For large scenes the querying of "host.ls()" can be relatively slow
# e.g. up to a second. Many instances calling it easily slows this
# down. As such, we cache it so we trigger it only once.
# todo: Instead of hidden cache make "CollectContainers" plug-in
cache_key = "__cache_containers"
scene_containers = instance.context.data.get(cache_key, None)
if scene_containers is None:
# Query the scenes' containers if there's no cache yet
host = registered_host()
scene_containers = list(host.ls())
for container in scene_containers:
# Embed the members into the container dictionary
container_members = set(get_container_members(container))
container["_members"] = container_members
instance.context.data["__cache_containers"] = scene_containers

User and group query is expensive operation, this will speed up publishing of multiple instances
@@ -62,8 +62,16 @@ def process(self, instance):
client = SlackPython3Operations(token, self.log)

if "@" in message:
if users is None:
cache_key = "__cache_ids"
Copy link
Collaborator

@BigRoy BigRoy Jan 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
cache_key = "__cache_ids"
cache_key = "__cache_ids"

Note that the context data is accessible by all plug-ins. It might be useful to avoid clashing with anything else that'll try to cache ids or alike like this.

Would: self.__class__.__name__ + "__cache_ids" be a good candidate? or at least "__slack_user_cache"?

@kalisp kalisp requested a review from 64qam January 16, 2023 16:43
@kalisp kalisp merged commit fa92f9e into release/3.15.x Jan 16, 2023
@kalisp kalisp deleted the bugfix/slack_user_groups_speedup branch January 16, 2023 17:40
@github-actions github-actions bot added this to the next-minor milestone Jan 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants