OCPERT-9 Cache slack user/group IDs in runtime #357

jhuttana · 2024-11-15T13:06:36Z

Could you please review the changes?

…the slack server Signed-off-by: Jayashree Huttanagoudar <[email protected]>

jhuttana · 2024-11-15T13:07:16Z

rioliu-rh · 2024-11-15T13:18:16Z

oar/core/notification.py

@@ -14,7 +14,7 @@


 logger = logging.getLogger(__name__)
-
+in_memory_cache = dict()


@jhuttana it's better to declare cache object as field of SlackClient, the class can be initialized in other module

rioliu-rh · 2024-11-15T13:30:19Z

oar/core/notification.py

@@ -265,11 +265,15 @@ def get_user_id_by_email(self, email):
            str: slack user id
        """
        email = self.transform_email(email)
+
+        if in_memory_cache.get(email) is not None:
+            return "<@%s>" % in_memory_cache.get(email)


@jhuttana this line has duplicate pattern declaration, suggest to declare local var userid, fill the var by if/else logic

userid = None if email in cache_dict: userid = cache_dict.get(email) else: // api call userid = resp["user"]["id"] cache_dict[email] = userid return "<@%s>" % userid

it looks more efficient

also, we can add debug level log here to indicate the contact is retrieved from cache. so if you execute the cmd with option -v/--debug, the logic can be traced.

rioliu-rh · 2024-11-15T13:31:29Z

oar/core/notification.py

@@ -287,6 +291,8 @@ def get_group_id_by_name(self, name):
            group id: slack group id
        """
        ret_id = ""
+        if in_memory_cache.get(name) is not None:


@jhuttana same concern as above

jhuttana · 2024-11-15T14:50:41Z

Thanks for the comments Rio. I will address these comments.

Signed-off-by: Jayashree Huttanagoudar <[email protected]>

rioliu-rh · 2024-11-18T11:04:52Z

oar/core/notification.py

@@ -238,6 +237,8 @@ def send_email(self, to_addrs, subject, content):


 class SlackClient:
+    cache_dict = dict()


@jhuttana cache object should be moved to class constructor __init__ like self.cache = {}, all subsequent calls need to be changed as well. e.g. self.cache[xx]=y

That would be perfect. I will do that.

…cache variable to the constructor Signed-off-by: Jayashree Huttanagoudar <[email protected]>

Signed-off-by: Jayashree Huttanagoudar <[email protected]>

rioliu-rh · 2024-11-19T10:39:00Z

oar/core/notification.py

@@ -239,6 +238,7 @@ def send_email(self, to_addrs, subject, content):

 class SlackClient:
    def __init__(self, bot_token):
+        self.cache_dict = dict()


can we do code formatting, put all the local vars together

self.a = xx self.b = yy

you can use formatter ms-python.black-formatter

I will check that and try to apply

For me it looks like declaring and initializing the runtime cache variable inside the constructor is fine (though it is referenced in two different methods). I don't see any difference in moving its declaration to the respective methods. For eg:

#Case-1: Declare and initialize the variable in constructor class test: def __init__(self): self.attr1 = dict() def a(self): self.attr1 = {1:'one'} print(self.attr1) def b(self): self.attr1 = {2:'two'} print(self.attr1) #Case-2: Declare and initialize the variable in respective methods class test1: def __init__(self): print("Inside test1 class") def aa(self): self.attr1 = dict() self.attr1 = {1:'one'} print(self.attr1) def bb(self): self.attr1 = dict() self.attr1 = {2:'Two'} print(self.attr1) #For Case-1 t = test() t.a() t.b() #For Case-2 tt = test1() tt.aa() tt.bb()

From your comment my understanding is that you are suggesting to go for Case-2 in the above example.
Though Case-1 and Case-2 are trying to achieve the same thing , Case-1 looks more efficient to me.
If my understanding is wrong could you please clarify what exactly are you suggesting?

I just want to make the code looks clean, put the all the local var declaration code together.

Suggested change

self.cache_dict = dict()

if not bot_token:

raise NotificationException("slack bot token is not available")

self.client = WebClient(token=bot_token)

self.cache_dict = dict()

Ooops sorry !! I totally understood it in a different way :)
I will do that.

rioliu-rh · 2024-11-19T10:45:31Z

oar/core/notification.py


+        logger.debug(f"Cached email ids: {self.cache_dict}")


@jhuttana we should not output all the cached contacts at func level. it will cause trace log flood, my suggestion is add debug log entry for cache put/get ops. then the user can see which contact is cached and which one is retrieved from cache

I think I am not getting it.
Do you mean to change it as: logger.debug(f"Cached email ids: {self.cache_dict.get(email)}") ?

if email in cache: // get userid from cache logger.debug(f"slack user id of {email} is retrieved from cached") else: // get userid from api call // put the userid to cache object logger.debug(f"slack user id of {email} is cached")

rioliu-rh · 2024-11-19T10:45:58Z

oar/core/notification.py


        if not ret_id:
            raise NotificationException(
                f"cannot find slack group id by name {name}")

+        logger.debug(f"Cached group ids: {self.cache_dict}")


@jhuttana same concern as above https://github.com/openshift/release-tests/pull/357/files#r1848102656

…rom the cache or added to the cache Signed-off-by: Jayashree Huttanagoudar <[email protected]>

Signed-off-by: Jayashree Huttanagoudar <[email protected]>

rioliu-rh · 2024-11-20T07:12:36Z

I think you need to create a new unit test to run funcs get_group_id_by_name/get_user_id_by_email multiple times to verify.
sample test case:

release-tests/tests/test_notification.py

Line 24 in 35262b0

def test_get_slack_user_id(self):

enabled debug logging:

release-tests/tests/test_artifacts.py

Lines 9 to 13 in 35262b0

    
           logging.basicConfig( 
        
               format="%(asctime)s: %(levelname)s: %(message)s", 
        
               datefmt="%Y-%m-%dT%H:%M:%SZ", 
        
               level=logging.DEBUG 
        
           )

jhuttana · 2024-11-20T07:49:44Z

I think you need to create a new unit test to run funcs get_group_id_by_name/get_user_id_by_email multiple times to verify. sample test case:

release-tests/tests/test_notification.py

Line 24 in 35262b0

def test_get_slack_user_id(self):

enabled debug logging:

release-tests/tests/test_artifacts.py

Lines 9 to 13 in 35262b0

logging.basicConfig(

format="%(asctime)s: %(levelname)s: %(message)s",

datefmt="%Y-%m-%dT%H:%M:%SZ",

level=logging.DEBUG

)

Sure I will give it a try

Signed-off-by: Jayashree Huttanagoudar <[email protected]>

rioliu-rh · 2024-11-21T03:01:31Z

/retest

rioliu-rh · 2024-11-21T09:24:11Z

/retest

jhuttana · 2024-11-21T13:23:05Z

/retest

rioliu-rh · 2024-11-22T06:11:43Z

you need to enable debug level logging in module test_notification.py, so you can see whether the user/group id can be retrieved from cache

jhuttana · 2024-11-22T06:41:00Z

you need to enable debug level logging in module test_notification.py, so you can see whether the user/group id can be retrieved from cache

Sure will do that

rioliu-rh · 2024-11-22T06:42:30Z

you need to enable debug level logging in module test_notification.py, so you can see whether the user/group id can be retrieved from cache

Sure will do that

if unit test is pass, we can merge this PR

Signed-off-by: Jayashree Huttanagoudar <[email protected]>

jhuttana · 2024-11-22T08:58:55Z

Verify the unittest case

$ python -m unittest test_notification.TestNotificationManager.test_get_slack_user_id_from_cache
.
----------------------------------------------------------------------
Ran 1 test in 5.943s

OK

$ python -m unittest test_notification.TestNotificationManager.test_get_slack_group_id_from_cache
.
----------------------------------------------------------------------
Ran 1 test in 9.199s

OK

Signed-off-by: Jayashree Huttanagoudar <[email protected]>

openshift-ci · 2024-11-22T09:47:19Z

@jhuttana: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

rioliu-rh · 2024-11-22T09:48:34Z

/lgtm
/approve

openshift-ci · 2024-11-22T09:49:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rioliu-rh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rioliu-rh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Cache slack user/group IDs in runtime to avoid duplicate requests to …

20f7d1a

…the slack server Signed-off-by: Jayashree Huttanagoudar <[email protected]>

openshift-ci bot requested review from barboras7 and cardelearht November 15, 2024 13:06

openshift-ci bot requested a review from rioliu-rh November 15, 2024 13:07

jhuttana marked this pull request as draft November 15, 2024 13:07

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 15, 2024

rioliu-rh reviewed Nov 15, 2024

View reviewed changes

Address review comments

57c1804

Signed-off-by: Jayashree Huttanagoudar <[email protected]>

jhuttana marked this pull request as ready for review November 18, 2024 08:50

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 18, 2024

openshift-ci bot requested a review from ltroiano-rh November 18, 2024 08:50

rioliu-rh reviewed Nov 18, 2024

View reviewed changes

jhuttana added 2 commits November 19, 2024 12:49

Address review comments to add debug logging and include the runtime …

b8b9568

…cache variable to the constructor Signed-off-by: Jayashree Huttanagoudar <[email protected]>

Correction to the logger instruction in the previous commit

82ec3f9

Signed-off-by: Jayashree Huttanagoudar <[email protected]>

rioliu-rh reviewed Nov 19, 2024

View reviewed changes

jhuttana added 2 commits November 20, 2024 00:30

Add proper logger message to indicate whether the value is retrived f…

e0e451a

…rom the cache or added to the cache Signed-off-by: Jayashree Huttanagoudar <[email protected]>

Put local variable together inside the constructor

568835b

Signed-off-by: Jayashree Huttanagoudar <[email protected]>

Add unit test cases for runtime cache to store user/group ids

cfe2258

Signed-off-by: Jayashree Huttanagoudar <[email protected]>

Update the group-id list in the test case

a954573

Signed-off-by: Jayashree Huttanagoudar <[email protected]>

Add debugging log config to the test file

9573c7e

Signed-off-by: Jayashree Huttanagoudar <[email protected]>

openshift-ci bot assigned rioliu-rh Nov 22, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 22, 2024

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 22, 2024

openshift-merge-bot bot merged commit 6eeb77f into openshift:master Nov 22, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPERT-9 Cache slack user/group IDs in runtime #357

OCPERT-9 Cache slack user/group IDs in runtime #357

jhuttana commented Nov 15, 2024

jhuttana commented Nov 15, 2024

rioliu-rh Nov 15, 2024

rioliu-rh Nov 15, 2024

rioliu-rh Nov 15, 2024

jhuttana Nov 18, 2024

rioliu-rh Nov 15, 2024

jhuttana commented Nov 15, 2024

rioliu-rh Nov 18, 2024 •

edited

Loading

jhuttana Nov 18, 2024

rioliu-rh Nov 19, 2024 •

edited

Loading

jhuttana Nov 19, 2024

jhuttana Nov 19, 2024

rioliu-rh Nov 19, 2024

jhuttana Nov 20, 2024

rioliu-rh Nov 19, 2024

jhuttana Nov 19, 2024

rioliu-rh Nov 19, 2024

rioliu-rh Nov 19, 2024

rioliu-rh commented Nov 20, 2024

jhuttana commented Nov 20, 2024

rioliu-rh commented Nov 21, 2024

rioliu-rh commented Nov 21, 2024

jhuttana commented Nov 21, 2024

rioliu-rh commented Nov 22, 2024

jhuttana commented Nov 22, 2024

rioliu-rh commented Nov 22, 2024

jhuttana commented Nov 22, 2024

openshift-ci bot commented Nov 22, 2024

rioliu-rh commented Nov 22, 2024

openshift-ci bot commented Nov 22, 2024

		@@ -14,7 +14,7 @@


		logger = logging.getLogger(__name__)

		in_memory_cache = dict()

		@@ -238,6 +237,8 @@ def send_email(self, to_addrs, subject, content):


		class SlackClient:
		cache_dict = dict()

-        self.cache_dict = dict()
+				if not bot_token:
+            raise NotificationException("slack bot token is not available")
+        self.client = WebClient(token=bot_token)
+        self.cache_dict = dict()

OCPERT-9 Cache slack user/group IDs in runtime #357

OCPERT-9 Cache slack user/group IDs in runtime #357

Conversation

jhuttana commented Nov 15, 2024

jhuttana commented Nov 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhuttana commented Nov 15, 2024

rioliu-rh Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rioliu-rh Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rioliu-rh commented Nov 20, 2024

jhuttana commented Nov 20, 2024

rioliu-rh commented Nov 21, 2024

rioliu-rh commented Nov 21, 2024

jhuttana commented Nov 21, 2024

rioliu-rh commented Nov 22, 2024

jhuttana commented Nov 22, 2024

rioliu-rh commented Nov 22, 2024

jhuttana commented Nov 22, 2024

openshift-ci bot commented Nov 22, 2024

rioliu-rh commented Nov 22, 2024

openshift-ci bot commented Nov 22, 2024

rioliu-rh Nov 18, 2024 •

edited

Loading

rioliu-rh Nov 19, 2024 •

edited

Loading