SD-838: timeouts #34
Merged
SD-838: timeouts #34
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hnnsgstfssn
commented
Oct 24, 2024
hnnsgstfssn
force-pushed
the
SD-838-timeouts
branch
from
bc3b222 to
26d12fc
Compare
hnnsgstfssn
commented
Oct 28, 2024
hnnsgstfssn
force-pushed
the
SD-838-timeouts
branch
from
26d12fc to
8287fcb
Compare
hnnsgstfssn
commented
Oct 28, 2024
|
It's not the best end state, but it was the smallest change I could come up with on short notice that should do what we wanted. More refactoring is needed in future. |
When the event handling takes to long the previous context is canceled and the request fails and the commit status ends up in a pending state. Using a separate context will allow the status to always be set, regardless of the event handling timing out.
Since it is very specific, might as well make it operate directly on labels. This should make it slightly clearer and easier to read. This reverts commit fcd2aeffd5b2752c7274514b7c78be7fc1bc60fd.
Oded-B
reviewed
Oct 28, 2024
hnnsgstfssn
force-pushed
the
SD-838-timeouts
branch
from
8287fcb to
dfcc28a
Compare
|
After a call we decided to make a few additional changes to log levels and returning and error, skipping continued execution when the config can not be read. |
|
Three test cases were tried manually:
|
Oded-B
previously approved these changes
Oct 28, 2024
This change refactors the event handling logic such that a deferred panic handler can log panics in the downstream handler logic. This should avoid crashing when such panics occur, and instead it would log the panic using the logger. Additionally, the parsing of the event payload to determine which handling logic to invoke is separated out, and now also indicates whether a match was found. This is to allow PR status updates to be applied once, when appropriate, and to enable ensuring that the success/failure update is always applied. To achieve this the individual downstream logic is factored out into separate functions, and errors encountered in them are returned where prHandleError were previously set. Getting the default branch and Telefonistka config is duplicated in each handler as needed.
The message seems to be informative only to developers i.e. better suited as a debug message. Moving it to the function that it is actually logging for ensures that it is always logged, instead of putting this burden on the caller.
Now that the earlier error is returned, the else is not needed and can be dropped, reducing the indentation of the happy path [1, 2]. [1] https://maelvls.dev/go-happy-line-of-sight/ [2] https://medium.com/@matryer/line-of-sight-in-code-186dd7cdea88
* Change metric type log level to debug Logging metric style info is better to handle properly; since this is one of very few instances it is changed to debug level for now. Future goal is to add tracing and metric instrumentation for such information. * Log event type once at start of handle function * Drop now duplicate log lines of the event type * Consistently add event type into PR logger Once PR logger includes the event_type. For consistency add the same field to the other PR loggers. * Remove now unused eventType argument
Prior when the configuration was not successfully fetched, the error was only logged but execution continued. Not fetching the configuration is an unrecoverable error that should result in upstream failure. Instead return the error to caller and let them log it.
hnnsgstfssn
force-pushed
the
SD-838-timeouts
branch
from
dfcc28a to
b17f800
Compare
Oded-B
approved these changes
Oct 28, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Recently some promotion pull requests have logged that they are reaching
the timeout of handling the request, causing updates of the Github
checks to get stuck in a pending state.
Follow commits for details.
It may be easier reviewing with an alternative diff tool, like difftastic (
brew install difftasticandGIT_EXTERNAL_DIFF=difft git diff main).Be sure to remove whitespace changes in the diff view.
Type of Change