Improves history handler error metrics and logs #5438
Merged
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
davidporter-id-au
changed the title
allenchen2244
approved these changes
Nov 1, 2023
taylanisikdemir
approved these changes
Nov 1, 2023
|
|
||
| } else if errors.As(err, &yarpcE) { | ||
|
|
||
| if yarpcE.Code() == yarpcerrors.CodeDeadlineExceeded { | ||
| scope.IncCounter(metrics.CadenceErrContextTimeoutCounter) | ||
| } | ||
| scope.IncCounter(metrics.CadenceFailures) |
There was a problem hiding this comment.
not related to your change: why do we increment both CadenceErrContextTimeoutCounter and CadenceFailures in this case? if there are dashboards looking at sum of these failures we will see extra numbers.
There was a problem hiding this comment.
I don't have context there to be honest
There was a problem hiding this comment.
@Groxx do we have dashboards summing these up and showing as total error count?
davidporter-id-au
added a commit
to davidporter-id-au/cadence
that referenced
this pull request
Dec 5, 2023
…flow#5438)" This reverts commit 4ece98a.
davidporter-id-au
added a commit
to davidporter-id-au/cadence
that referenced
this pull request
Dec 5, 2023
…flow#5438)" This reverts commit 4ece98a.
davidporter-id-au
added a commit
that referenced
this pull request
Dec 6, 2023
davidporter-id-au
added a commit
to davidporter-id-au/cadence
that referenced
this pull request
Dec 6, 2023
davidporter-id-au
added a commit
that referenced
this pull request
Dec 6, 2023
This was originally added (and not working) with #5438 and this followup corrects it and adds some actual metrics tests to ensure such a miss doesn't happen again. The driver of this change, to reiterate was two things: - More concretely, there are some times of invalid data are annoyingly difficult to track down because it lacks runID information. Obviously the oncall can dig around in the DB for the workflow and guess, but it's operationally quite a lot of work in a fast-moving environment. Some problems with invalid workflows without a current runID in this state blocked a preproduction environment for quite a while. - Zooming out, a goal @Groxx and others have had, is to make errors able to be much richer by wrapping them. However, this requires more than case-switching on types in order to convey more useful information such as the stacktrace and debug info. However, to do so requires any logic which does type or equality matching on errors to start properly using errors.Is/As. This is a small part of that initiative (albeit with a few bumps)
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.
What changed?
Some bad data caused a tasklist to get stuck while trying to update a workflow which was in a bad state. This was more difficult to identify than it should be because the RunID was missing information from the error logs. This change includes this additional information and also protects some of the metrics emission from wrapped errors being misclassified.
This is not an attempt to do all error handling / wrapping correctly, it's just an incremental change, of which many more are needed.
Why?
How did you test it?
Unit tests
Potential risks
Release notes
Documentation Changes