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

Improvement suggestion for error handling #55

Open
yeldiRium opened this issue Jan 20, 2025 · 0 comments
Open

Improvement suggestion for error handling #55

yeldiRium opened this issue Jan 20, 2025 · 0 comments

Comments

@yeldiRium
Copy link

Hi there,

thanks for making this great package. It aids me a lot in building my own
language server and I like it quite a bit.

However I have an improvement suggestion. I am currently trying to wrap the lps
server handler to add telemetry. With web servers and other kinds of servers you
would usually do this by wrapping the handler and recording some information
about its execution. This currently looks like this, where innerHandler is a
jsonrpc2.Handler as returned by protocol.ServerHandler(...).

func WrapInTelemetry(telemetry *Telemetry, innerHandler jsonrpc2.Handler) jsonrpc2.Handler {
	wrappedHandler := func(ctx context.Context, reply jsonrpc2.Replier, req jsonrpc2.Request) error {
		ctx, span := telemetry.Tracer.Start(ctx, req.Method())
		defer span.End()

		SetGlobalAttributes(span)

		err := innerHandler(ctx, reply, req)
		if err != nil {
			telemetry.logger.Sugar().Errorf("error occured and is being sent to telemetry!")
			span.SetStatus(codes.Error, "handler failed")
			span.RecordError(err)
			return err
		}

		span.SetStatus(codes.Ok, "handler succeeded")
		return nil
	}

	return wrappedHandler
}

Now I'm running into a problem: The inner handler does not propagate errors.

This is due the behavior in serverDispatch,
where all of the handlers handle errors by sending them to the client - which
makes sense - but then not propagating the error, but only returning an error
to the server, if sending the reply to the client failed.

This is probably done to keep the error handling localized and not expose
errors to the server that can be dealt with by informing the client about them.
However, if the error can't be handled by telling the client about it - and I
have some of these kinds of errors - then the server needs to be able to at
least know about them and log them.

Do you think there is a way to change this behavior without breaking everything?
Or is there an alternative way for my wrapper to know that an error occured?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

1 participant