-
Notifications
You must be signed in to change notification settings - Fork 504
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
add base concept of entities to signals #2977
Conversation
src/dispatch/entity/service.py
Outdated
db_session.commit() | ||
|
||
|
||
def get_cases_with_entity(db: Session, entity_id: int, days_back: int) -> int: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def get_cases_with_entity(db: Session, entity_id: int, days_back: int) -> int: | |
def get_cases_with_entity(db: Session, entity_id: int, days_back: int) -> list[Case]: |
|
||
@router.get("/{entity_id}/signal_instances", response_model=None) | ||
def get_signal_instances_by_entity(entity_id: int, db: Session = Depends(get_db)): | ||
instances = get_signal_instances_with_entity(db, entity_id, days_back=30) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to ship this with the 30 day default for now and then add a datetime select after mvp?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we worried about returning too much data? If so, we should limit the number of rows returned via limit
instead of picking some arbitrary date.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the days_back
was there to add support for a date range picker on the entity tab (not implemented yet). we probably do care about too much data though as well.
Global entity types are not implemented. I will remove from front-end, but keep it in the model. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, some small suggestions
# If a field was specified for this entity type, only search that field | ||
if not field or key == field: | ||
# Search the string for matches to the entity type's regular expression | ||
if match := entity_regex.search(val): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if we define a field but not a regex? Shouldn't we just return whatever is in the value of that field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now setting a regex is required. That's a good point though if we just want the whole value and know it's always going to be in a certain field in a certain signal. I guess right now you'd just set the field and the regex to .*
or a pattern for the value. We should probably support no regex though.
|
||
@router.get("/{entity_id}/signal_instances", response_model=None) | ||
def get_signal_instances_by_entity(entity_id: int, db: Session = Depends(get_db)): | ||
instances = get_signal_instances_with_entity(db, entity_id, days_back=30) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we worried about returning too much data? If so, we should limit the number of rows returned via limit
instead of picking some arbitrary date.
this.isLoading = false; | ||
}, | ||
methods: { | ||
async getSignalInstances() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you have to make these async? It's not something I've seen before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted the code above to be async and didn't have another non-async version of the getSignal... api call. We could add a non-async version probably.
thanks for looking at it |
Right now, the only type of Entity implemented will recursively search a signal for a entity that matches a regular expression (as opposed to specify, say the field/key name, where that entity should be). I will end up implementing both as an option. There is also the concept of a "global" entity, where we will search all signals, recursively, for that entity.
We should prioritize "time-to-first-render" and not let entity correlation be a bottleneck, although I expect this will still be fairly performant, all things considered. Only after a signal/case has been fully processed, will we correlate entities with other cases and signals. We can use UI tricks to try to make this invisible to the user and our UI is already set up well for this, by putting information with tabs on the CaseEditSheet.
Case View Mockup
Implemented Entity Case View
You can click the red context cards and view the signals. Cases table dialog not implemented yet.