Expose full API error messages to caller

[lrettig]

See my thread with @antonlerner at #2071 (review):

I feel that we should expose only a short, nonspecific error message via the API, and print the full error to the logs (this is how the API is currently implemented). Anton feels that we should expose the full error to the caller, especially for now since we're the only customers of the API.

Should we change this?

[avive]

It depends on which api service. Some api services main clients are wallets (is that what you mean by 'we' above?). The error from api services for wallets should be descriptive enough to inform users about what went wrong and the severity. I assume we want to report only critical errors via the api that requires the user to take action. For example, if there's a panic in a node's module and the node requires a restart then we want to display a modal with a short text message that doesn't have to be very detailed and give the user an option to restart the node or quit the app, but the full error technical details will be in the node's logs and will also be reported to us via telemetry for opted-in non-managed and for managed nodes. Also, the design for telemetry also accounts for managed nodes error reporting. See the telemetry smip. So, with this current design we don't rely on the api for full technical error reporting from nodes and the main goal is to inform user that there's a critical error that requires them to take action which is - restart the node or quit the app (when the wallet is configured to use that local managed node).

[avive]

So to clarify more, I believe that the Node Service should only report errors to the API which are critical in the sense that the node stopped working properly. e.g. a panic in the p2p module so wallet users can choose to restart (or go check if they have a new node with bug fixes they need to install by updating the app).

2nd client are backup agents and explore / dash backends - they also need to know if the managed node stopped working properly and needs restart.

For both type of clients, client can't do anything to resolve an error error, so it is useless to show them anything that does not require them to decide if to restart, quit or update their node software.

For all other cases such as errors that don't cause modules to stop and the node from functioning properly, there's telemetry and node logs. I can't think of an API client that will be interested in these. The telemetry service can be thought of a kind of an api client to a managed centralized error collecting store API...

[lrettig]

Sorry for not being clearer in my description. I think we're talking about two different things here. The ErrorStream endpoint in the NodeService streams all errors and panics as they occur (implementation here). I think this is what you're talking about.

What Anton and I were discussing is what happens when an error occurs inside the API itself, such as a failure finding some requested data. Here is one such example:

go-spacemesh/api/grpcserver/globalstate_service.go

Lines 436 to 440 in ce9d5da

	root, err := s.Mesh.GetLayerStateRoot(layer.Layer.Index())
	if err != nil {
	log.Error("error retrieving layer data: %s", err)
	return status.Errorf(codes.Internal, "error retrieving layer data")
	}

[avive]

Sorry for not being clearer in my description. I think we're talking about two different things here. The ErrorStream endpoint in the NodeService streams all errors and panics as they occur (implementation here). I think this is what you're talking about.

What Anton and I were discussing is what happens when an error occurs inside the API itself, such as a failure finding some requested data. Here is one such example:

go-spacemesh/api/grpcserver/globalstate_service.go

Lines 436 to 440 in ce9d5da

	root, err := s.Mesh.GetLayerStateRoot(layer.Layer.Index())
	if err != nil {
	log.Error("error retrieving layer data: %s", err)
	return status.Errorf(codes.Internal, "error retrieving layer data")
	}

i guess we can have short error messages and error code that client will display and a field w more tech details which is what goes to the logs so integration tests can output more detailed errors. The expanded tech details are not interesting for non technical users who use apps with data from the api.

[lrettig]

we can have short error messages and error code that client will display

agree, with one caveat: right now we're using this list of standard codes so there's a limit to how specific we can be with them. If necessary, we can fork, wrap, or otherwise extend this list.

[moshababo]

@lrettig still relevant?

[lrettig]

No, we don't want to change this behavior