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

api: blocks/{round}/logs endpoint #5865

Merged
merged 30 commits into from
Apr 30, 2024

Conversation

joe-p
Copy link
Contributor

@joe-p joe-p commented Dec 11, 2023

Summary

This is a new endpoint to get all of the logs from all appcalls in a given block. This endpoint was created specifically due to the encoding issue with the current /blocks/{round} endpoint (#5807).

Test Plan

Tests cover the following scenarios:

  • log in outer transaction
  • log from inner transaction
  • log from newly created application
  • logs from multiple outer app calls

daemon/algod/api/server/v2/handlers.go Outdated Show resolved Hide resolved
daemon/algod/api/server/v2/handlers.go Outdated Show resolved Hide resolved
daemon/algod/api/server/v2/handlers.go Outdated Show resolved Hide resolved
Copy link

codecov bot commented Dec 11, 2023

Codecov Report

Attention: Patch coverage is 0% with 40 lines in your changes are missing coverage. Please review.

Project coverage is 56.17%. Comparing base (5f81b7b) to head (267de9e).
Report is 1 commits behind head on master.

Files Patch % Lines
daemon/algod/api/server/v2/handlers.go 0.00% 35 Missing ⚠️
libgoal/libgoal.go 0.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5865      +/-   ##
==========================================
+ Coverage   52.42%   56.17%   +3.75%     
==========================================
  Files         482      482              
  Lines       67969    68009      +40     
==========================================
+ Hits        35630    38205    +2575     
+ Misses      29656    27210    -2446     
+ Partials     2683     2594      -89     

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

@joe-p
Copy link
Contributor Author

joe-p commented Dec 11, 2023

I've verified the logs are returned properly via external testing here: https://github.com/joe-p/block-log-debug/blob/6daea7ad53b83366c75aaf3325c719bc6fbd0646/__test__/block-log-debug.test.ts#L12

I'll need to write the proper tests here but should be good to go.

Other things to consider:

  1. Do we want query parameters to filter logs? I can imagine a prefix and appID filter would be useful.
  2. I feel like we should probably have some sort of way to limit response size. I think the most reasonable approach is to add query parameters for start and maxResults parameter but this will only work if txn order is deterministic in the block. I would think it is but would like some confirmation.

@jannotti
Copy link
Contributor

Since this returns, at most, the logs in one block, I don't think you need to add any filtering or limiting.

@joe-p
Copy link
Contributor Author

joe-p commented Dec 12, 2023

Yeah true, we know the upper bound due to the block size limit. Keeping it as is is good with me then.

Copy link
Contributor

@jannotti jannotti left a comment

Choose a reason for hiding this comment

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

A few small things you may or may not bother with.

daemon/algod/api/algod.oas2.json Outdated Show resolved Hide resolved
daemon/algod/api/server/v2/handlers.go Outdated Show resolved Hide resolved
daemon/algod/api/server/v2/handlers.go Outdated Show resolved Hide resolved
@joe-p joe-p force-pushed the feat/blocks_log_endpoint branch from 5971300 to 8c3ae7e Compare January 19, 2024 21:38
@joe-p joe-p marked this pull request as ready for review January 19, 2024 21:40
@joe-p
Copy link
Contributor Author

joe-p commented Jan 19, 2024

@jasonpaulos @jannotti I just added a fairly basic test in test/e2e-go/restAPI/other/appsRestAPI_test.go. Seemed like the most appropriate place for this to go but let me know what else is needed before this can be merged.

test/e2e-go/restAPI/other/appsRestAPI_test.go Outdated Show resolved Hide resolved
test/e2e-go/restAPI/other/appsRestAPI_test.go Outdated Show resolved Hide resolved
test/e2e-go/restAPI/other/appsRestAPI_test.go Outdated Show resolved Hide resolved
daemon/algod/api/server/v2/handlers.go Outdated Show resolved Hide resolved
daemon/algod/api/server/v2/handlers.go Outdated Show resolved Hide resolved
joe-p and others added 2 commits February 23, 2024 14:17
Co-authored-by: Jason Paulos <jasonpaulos@users.noreply.github.com>
jasonpaulos
jasonpaulos previously approved these changes Mar 5, 2024
@jasonpaulos jasonpaulos requested a review from jannotti March 7, 2024 21:16
daemon/algod/api/algod.oas2.json Outdated Show resolved Hide resolved
daemon/algod/api/algod.oas2.json Show resolved Hide resolved
daemon/algod/api/server/v2/handlers.go Outdated Show resolved Hide resolved
daemon/algod/api/server/v2/handlers.go Outdated Show resolved Hide resolved
daemon/algod/api/server/v2/handlers.go Outdated Show resolved Hide resolved
test/e2e-go/restAPI/other/appsRestAPI_test.go Show resolved Hide resolved
@joe-p joe-p closed this Apr 26, 2024
@joe-p joe-p reopened this Apr 26, 2024
jannotti
jannotti previously approved these changes Apr 29, 2024
Copy link
Contributor

@jannotti jannotti left a comment

Choose a reason for hiding this comment

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

I'm happy if we get some form of the description change I recommended. I want the response order to be defined.

daemon/algod/api/algod.oas2.json Outdated Show resolved Hide resolved
Copy link
Contributor

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

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

+1 to being more explicit about the endpoint description, but other than that, this looks good

Co-authored-by: John Jannotti <john.jannotti@algorand.com>
@jasonpaulos jasonpaulos requested a review from jannotti April 30, 2024 14:13
@jasonpaulos jasonpaulos merged commit 13c50af into algorand:master Apr 30, 2024
18 checks passed
@joe-p joe-p deleted the feat/blocks_log_endpoint branch April 30, 2024 16:00
@gmalouf
Copy link
Contributor

gmalouf commented May 14, 2024

closes #5807

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

/blocks/{round} endpoint returning malformed logs (non-ASCII data only)
4 participants