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

Decouple evaluation logs from aggregations #230

Merged

Conversation

andresgutgon
Copy link
Contributor

@andresgutgon andresgutgon commented Sep 19, 2024

What?

Part of this issue #205 this PR decouples big numbers aggregation from logs query so we can paginate (maybe) the logs query.

image

TODO

  • Aggregation query for counters + tests
  • Aggregation for mean query
  • Aggregation for modal query
  • Fix queries
  • Add limit of 1000 to evaluation results table now that's decouple from stats.
  • Ask product if showing always total costs is what we want ❓
  • Server Action for using totals query in browser storage (disable polling, only on mount)
  • Server Action for using mean query in browser storage (disable polling, only on mount)
  • Server Action for using modal query in browser storage (disable polling, only on mount)
  • Mix table and client now that is decoupled

Next PR

  • Add new rows in the table when a websocket arrive for a new processed evaluation result log.
  • Update stats when evaluation is finished. We know this via WebSockets.

Fancy logs selection by document version for meanQuery and modalQuery

  • Carlos's idea. Talk more offline and probably do it in a separate PR

@andresgutgon andresgutgon added the 🚧 wip Work in progress label Sep 19, 2024
@andresgutgon andresgutgon force-pushed the feature/decouple-evaluation-logs-from-aggregations branch from 8160dff to caf8ca7 Compare September 19, 2024 15:41
and(
eq(evaluationResultsScope.evaluationId, evaluation.id),
eq(documentLogsScope.documentUuid, documentUuid),
getCommitFilter(commit),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implement filtering by all the versions of the documents since it was changed until the version of the document is in the current commit in the screen.

I need to talk more offline about this. Not really sure.

I think this way keeps current behaviour

@andresgutgon andresgutgon force-pushed the feature/decouple-evaluation-logs-from-aggregations branch 2 times, most recently from 9d65427 to 94a0ae7 Compare September 20, 2024 08:13
@andresgutgon andresgutgon removed the 🚧 wip Work in progress label Sep 20, 2024
@andresgutgon andresgutgon force-pushed the feature/decouple-evaluation-logs-from-aggregations branch 4 times, most recently from 3f7178c to d714839 Compare September 20, 2024 12:34
{
fallbackData: serverData,
},
)
Copy link
Collaborator

@geclos geclos Sep 20, 2024

Choose a reason for hiding this comment

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

add the interval refetch every x seconds please, we need it until we implement realtime streaming of logs

Copy link
Contributor

Choose a reason for hiding this comment

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

We can manually refetch with websockets, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

ah you mean from actual real logs, not only from batches

Copy link
Collaborator

Choose a reason for hiding this comment

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

we can do it from websockets but until that is implemented i don't want to break the realtime feeling of this section

refreshInterval: 0,
fallbackData: modal,
},
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto for all of these

</Text.H4M>
</div>
),
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

cool! 👌🏼

geclos
geclos previously approved these changes Sep 20, 2024
Copy link
Collaborator

@geclos geclos left a comment

Choose a reason for hiding this comment

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

looks cool 👌🏼

csansoon
csansoon previously approved these changes Sep 20, 2024
@andresgutgon andresgutgon dismissed stale reviews from csansoon and geclos via f34996c September 20, 2024 13:48
@andresgutgon andresgutgon force-pushed the feature/decouple-evaluation-logs-from-aggregations branch 2 times, most recently from f34996c to b72bab7 Compare September 20, 2024 13:58
@andresgutgon andresgutgon force-pushed the feature/decouple-evaluation-logs-from-aggregations branch from b72bab7 to f35f84a Compare September 20, 2024 14:10
@andresgutgon andresgutgon merged commit 19e99e0 into main Sep 20, 2024
4 checks passed
@andresgutgon andresgutgon deleted the feature/decouple-evaluation-logs-from-aggregations branch September 20, 2024 14:18
@github-actions github-actions bot locked and limited conversation to collaborators Sep 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants