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

Allow to choose what information to include in HTTP/2 GOAWAY frame's debugData #5160

Open
trustin opened this issue Apr 18, 2016 · 9 comments
Labels

Comments

@trustin
Copy link
Member

trustin commented Apr 18, 2016

Related: line/armeria#145

It seems like the debugData in our HTTP/2 GOAWAY frame includes only the exception message. It is sometimes not enough because some exceptions do not have a message. It would be at least very useful if debugData contains the type of the exception as well. That is, we should use String.valueOf(exception) instead of exception.getMessage().

Also, in some cases, an Http2Exception can have a non-null cause. i.e. http2Ex.getCause() != null It would be also very useful to get the type and the message of the cause as well as the Http2Exception itself.

Although not always desirable, for some error types such as INTERNAL_ERROR(0x2), a user might also want to get the stack trace of the Http2Exception and its cause.

Perhaps we could make this pluggable so that we provide a few sensible 'debug data generator' implementations and let users write their own ones.

@trustin
Copy link
Member Author

trustin commented Apr 18, 2016

/cc @Scottmitch @nmittler

@nmittler
Copy link
Member

Would sending a stacktrace meet your needs in general? If so, it would seem that we could just have 2 implementations:

  1. what we do today (doesn't leak internals of the server. Should probably be the default)
  2. debugging stacktrace

@trustin
Copy link
Member Author

trustin commented Apr 19, 2016

what we do today

plus the type name of the exception? (both for http2Ex and cause)

@Scottmitch
Copy link
Member

Scottmitch commented Apr 19, 2016

@trustin @nmittler - Agreed we should not dump a stacktrace by default. Including the type name of the exception(s) sgtm.

@nmittler
Copy link
Member

@Scottmitch @trustin one other thing to consider: we may want the strategy selection to be per-request. If sitting behind a proxy, we may get a mix of requests - only some of which allow the full stack trace (e.g. a magic header is added by the proxy if it was determined the request to be safe).

@Scottmitch
Copy link
Member

@nmittler - This sounds potentially useful, maybe we make a method which generates the debug data overridable and make sure it has the necessary information (streamId, exceptions, etc...)? The application can then implement w/e logic they want.

@nmittler
Copy link
Member

@Scottmitch probably just allowing a user to provide a strategy when creating the pipeline should be sufficient. We can provide 2 simple (non-request-based) implementations for stacktraces or no stacktraces. The application developers can write their own that uses these based on some trigger (e.g. magic header).

@Scottmitch
Copy link
Member

Scottmitch commented Apr 19, 2016

Default strategies not based on stream specific data sound good to me. I guess I was confused about the we may want the strategy selection to be per-request. because the issue is discussing GO_AWAY. Seems strange to have an individual request impact the behavior of GO_AWAY which is more a connection level thing. I was thinking the type of policy you mentioned (magic header) may be more applicable to a RST_STREAM type exception. Here it would be useful to have a Http2Stream or streamId ... otherwise how will they be able to be able to apply a specific strategy based upon something that is stream specific (magic headers).

@nmittler
Copy link
Member

yes that's true ... GO_AWAY is for the entire connection, so I guess my comment about stream-specific strategy doesn't really apply.

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

No branches or pull requests

3 participants