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

rework on logGrpcErrorAndReportEvent #8

Open
Madhu-1 opened this issue Jan 26, 2023 · 0 comments
Open

rework on logGrpcErrorAndReportEvent #8

Madhu-1 opened this issue Jan 26, 2023 · 0 comments

Comments

@Madhu-1
Copy link
Member

Madhu-1 commented Jan 26, 2023

The fact there's an And in the function name immediately flags that this should be split. Looking at it deeper, the first part of the function is just a selection of strings, and the actual work of the function is two lines. Similarly, only one of these groups of lines is ever used more than once.

I can kind of understand want to have a single place to abstract error reporting, but I don't think this is really necessary. Given that this is only even used in three places, that also indicates this is probably needless abstraction. I could still see an argument for making it easy to write and reference our error reporting strings, so it might make sense to just define some struct like this, and define a few of these at the top of some file:

type struct StorageClientGrpcEvent {
    msg string
    eventReason string
    eventType string
    errorCode codes.Code
}

Then you could go to the three places this function is currently used and replace it with the following, which avoids us falling into the trap of constraining ourselves to an abstraction format we might not consistently need:

		if st, ok := status.FromError(err); ok {
			var errStruct = SomeVarName
			s.Log.Error(err, "StorageProvider:"+grpcCallName+":"+errStruct.msg)
			s.recorder.ReportIfNotPresent(instance, errStruct.eventType, errStruct.eventReason, errStruct.msg)
		}
		return reconcile.Result{}, fmt.Errorf(errStruct.msg, err)

If you agree with this assessment, this can also come in as a future PR from you or someone else. And don't take these examples as design, this was just off the top of my head. If you don't, that's valid, let's hear why.

Originally posted by @jarrpa in #7 (comment)

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

No branches or pull requests

1 participant