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

feat(workflow_engine): Add process_data_packets method #82002

Merged
merged 7 commits into from
Dec 13, 2024

Conversation

saponifi3d
Copy link
Contributor

Description

Adding a wrapper method, process_data_packets to merge the process_data_sources and process_detectors.

Still thinking through higher level abstractions to simplify using the workflow_engine, but this should simplify integrations a little further for now.

@saponifi3d saponifi3d requested a review from a team December 12, 2024 02:25
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Dec 12, 2024
@saponifi3d saponifi3d force-pushed the jcallender/aci/process_data_packets branch from 923a9fc to 623b1e8 Compare December 12, 2024 02:40
Copy link

codecov bot commented Dec 12, 2024

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
23307 1 23306 211
View the top 1 failed tests by shortest run time
tests.sentry.workflow_engine.processors.test_workflow.TestProcessWorkflows::test_error_event
Stack Traces | 4s run time
#x1B[1m#x1B[.../workflow_engine/processors/test_workflow.py#x1B[0m:30: in test_error_event
    assert triggered_workflows == {self.error_workflow}
#x1B[1m#x1B[31mE   AssertionError: assert set() == {<Workflow at...211316527108>}#x1B[0m
#x1B[1m#x1B[31mE     #x1B[0m
#x1B[1m#x1B[31mE     Extra items in the right set:#x1B[0m
#x1B[1m#x1B[31mE     <Workflow at 0x7efdca5be180: id=12, name='error_workflow', organization_id=4555211316527108>#x1B[0m
#x1B[1m#x1B[31mE     #x1B[0m
#x1B[1m#x1B[31mE     Full diff:#x1B[0m
#x1B[1m#x1B[31mE     + set()#x1B[0m
#x1B[1m#x1B[31mE     - {#x1B[0m
#x1B[1m#x1B[31mE     -     <Workflow at 0x7efdca5be180: id=12, name='error_workflow', organization_id=4555211316527108>,#x1B[0m
#x1B[1m#x1B[31mE     - }#x1B[0m

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

query_id = BoundedBigIntegerField()

# TODO - Add a type here
Copy link
Contributor

Choose a reason for hiding this comment

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

Colleen creates an enum for this in #81953

**kwargs,
) -> tuple[Workflow, Detector, DetectorWorkflow, DataConditionGroup]:
"""
Create a Worfkllow, Detector, DetectorWorkflow, and DataConditionGroup for testing.
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't normally nitpick typos like this, but since some tests are failing anyway I thought I would point out this docstring 😅

Copy link
Member

@ceorourke ceorourke left a comment

Choose a reason for hiding this comment

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

lgtm, nice to bundle these together

Comment on lines +8 to +10
def process_data_packets(
data_packets: list[DataPacket], query_type: str
) -> list[tuple[Detector, dict[DetectorGroupKey, DetectorEvaluationResult]]]:
Copy link
Member

Choose a reason for hiding this comment

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

Is this idea that the snuba subscription result consumer will call this with each packet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! That said, it could also work as a batch processor since it takes a list of packets.

@saponifi3d saponifi3d force-pushed the jcallender/aci/process_data_packets branch from 39105c8 to 1cdccbf Compare December 13, 2024 21:24
@saponifi3d saponifi3d force-pushed the jcallender/aci/process_data_packets branch from 1cdccbf to 881615d Compare December 13, 2024 21:59
Base automatically changed from jcallender/post_process_workflow_engine to master December 13, 2024 22:28
@saponifi3d saponifi3d force-pushed the jcallender/aci/process_data_packets branch from 881615d to 7d88677 Compare December 13, 2024 22:30
@saponifi3d saponifi3d enabled auto-merge (squash) December 13, 2024 23:34
@saponifi3d saponifi3d merged commit 3273e16 into master Dec 13, 2024
47 checks passed
@saponifi3d saponifi3d deleted the jcallender/aci/process_data_packets branch December 13, 2024 23:52
evanh pushed a commit that referenced this pull request Dec 17, 2024
## Description
Adding a wrapper method, `process_data_packets` to merge the
process_data_sources and process_detectors.

Still thinking through higher level abstractions to simplify using the
workflow_engine, but this should simplify integrations a little further
for now.
@github-actions github-actions bot locked and limited conversation to collaborators Dec 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants