Skip to content
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

Conditional creation of case resources for perf improvement on signal ingest #3331

Merged
merged 9 commits into from
May 4, 2023

Conversation

wssheldon
Copy link
Contributor

@wssheldon wssheldon commented May 1, 2023

Resources are still created successfully on case escalation

Screenshot 2023-05-01 at 3 34 46 PM

@wssheldon wssheldon added the enhancement New feature or request label May 1, 2023
@wssheldon wssheldon requested a review from mvilanova May 1, 2023 22:22
src/dispatch/case/flows.py Outdated Show resolved Hide resolved
src/dispatch/case/flows.py Outdated Show resolved Hide resolved
src/dispatch/case/flows.py Show resolved Hide resolved
src/dispatch/case/flows.py Show resolved Hide resolved
document=document, project_id=case.project.id, db_session=db_session
)
if create_resources:
case_create_resources_flow(db_session=db_session, case_id=case.id)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we move the code for creating a conversation (that doesn't leverage the functions in the conversation.flows module) inside case_create_resources_flow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was wondering that but I wasn't sure. It's core to the workflow and if we moved it to case_create_resources_flow it would be too binary. We have the conversation_target flag in the Signal, which is our conditional for creating a conversation or not.

        if conversation_target:
            try:
                # TODO: Refactor conversation creation using conversation_flows module
                conversation = create_conversation(case, conversation_target, db_session)
                conversation_in = ConversationCreate(

Also, we should probably use convo flow here or create a new flow for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the plan was to not create new conversations, but leveraging existing and configured ones in Dispatch for sending notifications and threading them. What am I missing?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we still need to create a conversation and this depends on individual_participants and team_participants, should these be passed as parameters to case_create_resources_flow to avoid fetching them twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought the plan was to not create new conversations, but leveraging existing and configured ones in Dispatch for sending notifications and threading them. What am I missing?

Only net new (non-deduplicated/snooze) Signals should ever enter the case_new_create_flow.

Filtered ones update the existing thread:

    # we don't need to continue if a filter action took place
    if signal_service.filter_signal(db_session=db_session, signal_instance=signal_instance):
        # If the signal was deduplicated, we can assume a case exists,
        # and we need to update the corresponding signal message
        if (
            signal_instance.filter_action == SignalFilterAction.deduplicate
            and signal_instance.case.signal_thread_ts  # noqa
        ):
            update_signal_message(
                db_session=db_session,
                signal_instance=signal_instance,
            )
        return signal_instance

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to pass the participants as args to create resource flow

src/dispatch/case/flows.py Outdated Show resolved Hide resolved
src/dispatch/individual/service.py Show resolved Hide resolved
src/dispatch/case/flows.py Outdated Show resolved Hide resolved
@wssheldon wssheldon merged commit 3812486 into master May 4, 2023
@wssheldon wssheldon deleted the enhancement/api-tuning branch May 4, 2023 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants