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

[442] - Pagination added for logs #832

Merged
merged 11 commits into from
Sep 3, 2024
Merged

[442] - Pagination added for logs #832

merged 11 commits into from
Sep 3, 2024

Conversation

huzaifmalik786
Copy link
Collaborator

No description provided.

@fatchat
Copy link
Contributor

fatchat commented Aug 25, 2024

depends on DalgoT4D/prefect-proxy#154

@fatchat fatchat requested a review from Ishankoradia August 25, 2024 15:40
@Ishankoradia
Copy link
Contributor

@fatchat @huzaifmalik786 is there a webapp PR coming for this.

@huzaifmalik786
Copy link
Collaborator Author

@Ishankoradia , it has already been raised. Please see it here:
DalgoT4D/webapp#1143

Thanks!

Copy link

codecov bot commented Aug 28, 2024

Codecov Report

Attention: Patch coverage is 9.52381% with 19 lines in your changes missing coverage. Please review.

Project coverage is 75.11%. Comparing base (44114f8) to head (dc951e7).
Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
ddpui/celeryworkers/tasks.py 0.00% 13 Missing ⚠️
ddpui/api/pipeline_api.py 0.00% 4 Missing ⚠️
ddpui/ddpprefect/prefect_service.py 33.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #832      +/-   ##
==========================================
- Coverage   75.22%   75.11%   -0.12%     
==========================================
  Files         184      184              
  Lines        7664     7678      +14     
==========================================
+ Hits         5765     5767       +2     
- Misses       1899     1911      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Ishankoradia
Copy link
Contributor

@huzaifmalik786 works nicely.

For the ai summary, can you make sure we are fetching all logs in the api v1/flow_runs/{flow_run_id}/logsummary.
It internally calls a function summarize_logs that in turn calls get_flow_run_logs_v2 . maybe replace this with a common function that fetches all pages with some configurable limit.

Thanks. Nice work.

@huzaifmalik786
Copy link
Collaborator Author

huzaifmalik786 commented Sep 2, 2024

@Ishankoradia , are you saying to use the same get_flow_run_logs function in the summarize_logs function?
I think get_flow_run_logs_v2 already satisfies our need. It returns all the logs without any pagination.

@Ishankoradia Ishankoradia self-requested a review September 3, 2024 11:29
@Ishankoradia Ishankoradia merged commit 8cddd37 into main Sep 3, 2024
1 of 4 checks passed
@Ishankoradia Ishankoradia deleted the dev_logs_pagination branch September 3, 2024 11:30
@fatchat fatchat mentioned this pull request Sep 12, 2024
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants