Capture servlet request parameters #4703
Conversation
laurit
requested review from
anuraaga,
iNikem,
jkwatson,
mateuszrzeszutek,
pavolloffay,
trask and
tylerbenson
as code owners
laurit
force-pushed
the
capture-servlet-request-parameters
branch
from
93f1823
to
663fb45
Compare
| @@ -16,6 +16,9 @@ class JettyServletHandlerTest extends AbstractServlet5Test<Object, Object> { | |||
|
|
|||
| @Override | |||
| String expectedServerSpanName(ServerEndpoint endpoint) { | |||
| if (ServerEndpoint.CAPTURE_PARAMETERS == endpoint) { | |||
| return "HTTP POST" | |||
There was a problem hiding this comment.
Why does this request have to be POST?
There was a problem hiding this comment.
It doesn't. I chose POST because GET parameters can be extracted from url and could also be captured on other http server frameworks. POST parameters, in contrast, are easy to capture with servlet api, but hard on all other http server frameworks. This is also the reason why it is only implemented for servlet.
There was a problem hiding this comment.
How important is it to capture parameters from application/x-www-form-urlencoded bodies vs limiting this feature to query parameters?
I think the application/x-www-form-urlencoded body support makes this feature harder to explain to users and (as you mentioned) harder to implement across all http server frameworks. I think it may also make this feature harder to upstream into the spec.
There was a problem hiding this comment.
I believe that from end users perspective it makes sense to capture a request parameter regardless of whether it was passed in url, post or in multipart request, because the same parameter could be passed by different means even in the same application. I suspect that it will be hard to explain why we capture a parameter only when it is passed in the url if we choose to ignore post parameters. Whilst it might be hard to get access to post or multipart parameters in most http server frameworks it would require addition effort to exclude these using servlet api.
I agree that specifying this will be hard because with some languages and frameworks it might be too hard to capture post parameters, while in other it will be easy and excluding them just to implement the feature the same way across languages and frameworks will feel wrong. If this is going to be specified it will need to allow flexibility to allow capturing extra info when it is available and not force capturing post parameters where it is hard.
There was a problem hiding this comment.
What do you think of renaming from "request parameters" to "query parameters"? (and still applying to both query strings and application/x-www-form-urlencoded bodies)
For some reason, when I see "request parameters", I'm not really sure what that means.
Though, I just went looking and I'm having a hard time finding a canonical definition for either "request parameter" or "query parameter" 😭 🤷♂️.
(I can live with "request parameters" if that's what others prefer, and of course the spec will have the last word on the naming..)
There was a problem hiding this comment.
Spring mvc uses @RequestParam. JAX-RS has @QueryParam (for parameters in query string) and @FormParam (for post).
* Capture servlet request parameters * use concurrenthashmap for cache
This pr implements capturing HTTP request parameters as span attributes when request is handled by servlet api. Similarly to capturing HTTP headers only parameters that are explicitly configured will be captured.