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

log ms into slot when getHeader/getPayload requests start #508

Merged
merged 4 commits into from
Jun 1, 2023
Merged

Conversation

metachris
Copy link
Collaborator

@metachris metachris commented May 13, 2023

📝 Summary

Log how late into the slot the getHeader and getPayload requests start.

This is particularly useful if proposers submit logs to debug the "too late request" response from the relay (i.e. when the relay received the request >4 sec into the slot). To be specific, this can tell whether the request started too late in the first place, or if it's the network between the proposer and relay

Moved the GetEnv* methods to a new common package, which also holds the default (mainnet) genesis time and slot time (12 sec). This removes the import of go-utils/cli across the package too!


✅ I have run these commands

  • make lint
  • make test-race
  • go mod tidy

@codecov-commenter
Copy link

codecov-commenter commented May 13, 2023

Codecov Report

Merging #508 (41aafa4) into develop (54eeaa2) will decrease coverage by 0.74%.
The diff coverage is 64.84%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@             Coverage Diff             @@
##           develop     #508      +/-   ##
===========================================
- Coverage    70.24%   69.50%   -0.74%     
===========================================
  Files            6        8       +2     
  Lines          541     1256     +715     
===========================================
+ Hits           380      873     +493     
- Misses         132      336     +204     
- Partials        29       47      +18     
Flag Coverage Δ
unittests 69.50% <64.84%> (-0.74%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cli/main.go 0.00% <0.00%> (ø)
cli/types.go 0.00% <0.00%> (ø)
server/mock_types.go 100.00% <ø> (ø)
server/types.go 70.00% <70.00%> (ø)
server/service.go 76.57% <74.14%> (+4.56%) ⬆️
server/utils.go 82.23% <84.80%> (+14.66%) ⬆️
server/mock_relay.go 88.44% <90.81%> (+5.58%) ⬆️
server/relay_entry.go 90.00% <100.00%> (+8.75%) ⬆️

... and 1 file with indirect coverage changes

@metachris metachris force-pushed the ts-log branch 2 times, most recently from b1a6ca0 to 1305e7f Compare May 13, 2023 09:01
@metachris metachris deleted the branch develop May 16, 2023 12:31
@metachris metachris closed this May 16, 2023
@metachris metachris reopened this May 17, 2023
@metachris metachris changed the base branch from main to develop May 17, 2023 13:59
@metachris metachris mentioned this pull request May 22, 2023
@metachris metachris changed the title submitBlindedBlock - log milliseconds into slot when request started log ms into slot when getHeader/getPayload requests start May 22, 2023
Makefile Show resolved Hide resolved
avalonche
avalonche previously approved these changes May 22, 2023
@metachris
Copy link
Collaborator Author

Added one more commit to log with Info level when the getPayload request starts. This will be useful when debugging bug reports like #519

Copy link
Collaborator

@michaelneuder michaelneuder left a comment

Choose a reason for hiding this comment

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

tiny comments, but LGTM

server/service.go Outdated Show resolved Hide resolved
server/service.go Outdated Show resolved Hide resolved
michaelneuder
michaelneuder previously approved these changes May 26, 2023
Copy link
Collaborator

@michaelneuder michaelneuder left a comment

Choose a reason for hiding this comment

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

tiny comments, but LGTM

server/service.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

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

makes sense and this PR in isolation generally makes sense

do we also want to log timings on the "backend" of each request, rather than just the "frontend" like we do in this PR?

@metachris
Copy link
Collaborator Author

do we also want to log timings on the "backend" of each request, rather than just the "frontend" like we do in this PR?

what do you mean with that?

@ralexstokes
Copy link
Collaborator

ralexstokes commented May 30, 2023

right now it just timestamps when the request starts, we could also log the time when the request finishes (the "backend")

e.g. add a log on 705 about finishing to match the log on 605 about starting

@metachris
Copy link
Collaborator Author

when the request finishes (the "backend")

this is not what's typically referred to as "the backend".

I don't think we need another log there, we already have a log when the payload was received from the relay, and at the end of the function, the HTTP middleware also logs the complete request summary

@metachris metachris mentioned this pull request Jun 1, 2023
Copy link
Collaborator

@michaelneuder michaelneuder left a comment

Choose a reason for hiding this comment

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

lgtm!

@metachris metachris merged commit 2a69b41 into develop Jun 1, 2023
@metachris metachris deleted the ts-log branch June 1, 2023 14:52
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.

5 participants