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

Improves metric and error handling for history #5469

Merged

Conversation

davidporter-id-au
Copy link
Contributor

@davidporter-id-au davidporter-id-au commented 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)

h := handlerImpl{
Resource: resource.NewTest(gomock.NewController(t), 0),
}
h.error(td.input, &scope, "some-domain", "some-wf", "some-run")
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not familiar with this mock library. where is the validation happening? AFAICT td.expectation is marking some method calls with once but when is it ensured those calls happened exactly once and no other calls?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't love the mocking library either, I'd like to get rid of it in favour of our normal gomock one, but it's what's already here, so I didn't try and change it.

    --- FAIL: TestCorrectUseOfErrorHandling/uncategorized_error (0.00s)
panic: 
assert: mock: I don't know what to return because the method call was unexpected.
	Either do Mock.On("IncCounter").Return(...) first, or remove the IncCounter() call.
	This method was unexpected:
		IncCounter(int)
		0: 1
	panic: 
assert: mock: I don't know what to return because the method call was unexpected.
	Either do Mock.On("IncCounter").Return(...) first, or remove the IncCounter() call.
	This method was unexpected:
		IncCounter(int)
		0: 1

You're right there's nothing ensuring there's a counter call, let me add an assertion on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added scope.Mock.AssertCalled below to ensure that completely ommiting a call to counter is not missed

@davidporter-id-au davidporter-id-au merged commit 018805b into uber:master Dec 6, 2023
16 checks passed
@davidporter-id-au davidporter-id-au deleted the bugfix/attempt-metrics-take-2 branch December 6, 2023 22:57
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.

2 participants