FEAT: Unified Notifications #290
Conversation
tahierhussain
changed the title
tahierhussain
requested review from
chandrasekharan-zipstack,
hari-kuriakose,
Deepak-Kesavan,
athul-rs,
harini-venkataraman,
nehabagdia,
johnyrahul,
muhammad-ali-e,
vishnuszipstack and
gaya3-zipstack
…ract into feat/unified-notifications
Signed-off-by: jagadeeswaran-zipstack <[email protected]>
…ract into feat/unified-notifications
| @@ -259,6 +260,8 @@ def make_user_organization_display_name(self, user_name: str) -> str: | |||
| return self.auth_service.make_user_organization_display_name(user_name) | |||
|
|
|||
| def user_logout(self, request: Request) -> Response: | |||
| session_id: str = request.COOKIES.get("sessionid") | |||
There was a problem hiding this comment.
NIT: We should use constants for these if we aren't already. Also I suggest following either camel case (sessionId) or snake case convention here (session_id)
There was a problem hiding this comment.
If your intention is to get sessionID -> you can use
request.session.session_key
If your intention is to get LOG_EVENTS_ID -> you can use
StateStore.get(Common.LOG_EVENTS_ID)
| # Get the Redis connection | ||
| r = get_redis_connection("default") | ||
|
|
||
| key_pattern = f"logs:{session_id}*" |
There was a problem hiding this comment.
NIT: This key creation / generation can be encapsulated in a function so that the push / retrieve from redis can follow the same logic. It'll be easier to maintain in case we wish to change this key later
| @@ -259,6 +260,8 @@ def make_user_organization_display_name(self, user_name: str) -> str: | |||
| return self.auth_service.make_user_organization_display_name(user_name) | |||
|
|
|||
| def user_logout(self, request: Request) -> Response: | |||
| session_id: str = request.COOKIES.get("sessionid") | |||
There was a problem hiding this comment.
If your intention is to get sessionID -> you can use
request.session.session_key
If your intention is to get LOG_EVENTS_ID -> you can use
StateStore.get(Common.LOG_EVENTS_ID)
backend/logs_helper/log_service.py
Outdated
|
|
||
| class LogService: | ||
| @staticmethod | ||
| def remove_logs_on_logout(session_id): |
There was a problem hiding this comment.
Add arguments Types and function return type
backend/logs_helper/log_service.py
Outdated
|
|
||
| if session_id: | ||
| # Get the Redis connection | ||
| r = get_redis_connection("default") |
There was a problem hiding this comment.
Can you re use CacheService from backend/utils/cache_service.py
There would have the cache functions. If not please add there.
There was a problem hiding this comment.
Done 👍
backend/logs_helper/log_service.py
Outdated
| key_pattern = f"logs:{session_id}*" | ||
|
|
||
| # Retrieve keys matching the pattern and delete them | ||
| keys = r.keys(key_pattern) |
There was a problem hiding this comment.
Cacheservice have a function clear_cache with key pattern. We can use that function here
There was a problem hiding this comment.
Got it
backend/logs_helper/views.py
Outdated
| @action(detail=False, methods=["get"]) | ||
| def get_logs(self, request): | ||
| # Extract the session ID | ||
| session_id: str = StateStore.get(LogsHelperKeys.LOG_EVENTS_ID) |
There was a problem hiding this comment.
I thing the variable should renamed as log_event_id.
If we need real session_id here , please use request.session.session_key
backend/logs_helper/views.py
Outdated
| """Store log message in Redis.""" | ||
| # Extract the session ID | ||
| logs_expiry = int(os.environ.get("LOGS_EXPIRATION_TIME_IN_SECOND", 3600)) | ||
| session_id: str = StateStore.get(LogsHelperKeys.LOG_EVENTS_ID) |
There was a problem hiding this comment.
like previous comments , change variable name
backend/logs_helper/views.py
Outdated
|
|
||
| redis_key = f"logs:{session_id}:{timestamp}" | ||
|
|
||
| self.r.setex(redis_key, logs_expiry, log) |
There was a problem hiding this comment.
insetad of this, can we use redis log? .. (I added the same comments somw where else too)
backend/logs_helper/views.py
Outdated
| return Response({"data": sorted_logs}, status=status.HTTP_200_OK) | ||
|
|
||
| @action(detail=False, methods=["post"]) | ||
| def store_log(self, request): |
There was a problem hiding this comment.
to know: Are we calling this method in every user logs?
There was a problem hiding this comment.
Yes, We call this method every time a something logged from frontend.
| @@ -137,3 +137,5 @@ ENABLE_LOG_HISTORY=True | |||
| LOG_HISTORY_CONSUMER_INTERVAL=30 | |||
| # Maximum number of logs to insert in a single batch. | |||
| LOGS_BATCH_LIMIT=30 | |||
| # Logs Expiry | |||
| LOGS_EXPIRATION_TIME_IN_SECOND=3600 | |||
There was a problem hiding this comment.
please add same in to django settings
There was a problem hiding this comment.
Got it
backend/logs_helper/views.py
Outdated
| class LogsHelperViewSet(viewsets.ModelViewSet): | ||
| """Viewset to handle all Tool Studio prompt related API logics.""" | ||
|
|
||
| r = get_redis_connection("default") |
There was a problem hiding this comment.
remove this and use cacheservice
| @@ -4,16 +4,11 @@ | |||
| from datetime import datetime, timezone | |||
| from typing import Any, Optional | |||
|
|
|||
| import redis | |||
| from django_redis import get_redis_connection | |||
There was a problem hiding this comment.
Can't do this. This pubsub_helper is core library package that we are using in worker. backend,etc.. Some of the services are not in django, and they don;t have the context of redis
There was a problem hiding this comment.
Got it 👍
| cls.r.publish(channel, log_data) | ||
|
|
||
| # Check if the payload type is "LOG" | ||
| if payload["type"] == "LOG": |
There was a problem hiding this comment.
It should n't be here. please move this in to consumer in backend.
Also, we have some changes in consumer too for design change of socket IO
| @@ -4,16 +4,11 @@ | |||
| from datetime import datetime, timezone | |||
There was a problem hiding this comment.
This package have some changes in another PR. that have design changes of socketio messages.
…ract into feat/unified-notifications
Signed-off-by: jagadeeswaran-zipstack <[email protected]>
for more information, see https://pre-commit.ci
…ract into feat/unified-notifications
for more information, see https://pre-commit.ci
…ract into feat/unified-notifications
frontend/src/components/custom-tools/display-logs/DisplayLogs.jsx
Outdated
Show resolved
Hide resolved
|
|
There was a problem hiding this comment.
@jagadeeswaran-zipstack remove this if unused
| return Response({"data": sorted_logs}, status=status.HTTP_200_OK) | ||
|
|
||
| @action(detail=False, methods=["post"]) | ||
| def store_log(self, request: HttpRequest) -> Response: |
There was a problem hiding this comment.
@jagadeeswaran-zipstack why do we have this API? When / how will FE be calling this?
There was a problem hiding this comment.
We're storing notifications from the frontend. For instance, when an adapter is created, we'll get a notification that says 'Added Successfully'. These messages are stored using this API.
What
Unified Notifications consists of the following changes:
Why
The unified notification changes were implemented to enhance UX. Users can now see log and notification messages in one place, and these messages are persisted for 1 hour.
How
Can this PR break any existing features. If yes please list of possible items. If no please exaplin why. (PS: Admins do not merge the PR without this section filled)
Yes, this PR can break existing features. The code changes have been made in quite a few existing modules, increasing the chances of regression.
Database Migrations
NA
Env Config
LOGS_EXPIRATION_TIME_IN_SECOND - This env variable stores the TTL in seconds. The default value is 3600 (1 hour).
Relevant Docs
NA
Related Issues or PRs
NA
Dependencies Versions
NA
Notes on Testing
NA
Screenshots
Checklist
I have read and understood the Contribution Guidelines.