Safely parse URL and respect http server status code error rule #5128
Conversation
amarziali
force-pushed
the
andrea.marziali/uri-tomcat
branch
from
6b43747
to
1df795e
Compare
BenchmarksParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 22 cases. See unchanged results
|
amarziali
force-pushed
the
andrea.marziali/uri-tomcat
branch
6 times, most recently
from
46ffec0
to
c8c6d7c
Compare
...java/datadog/trace/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientDecorator.java
Outdated
Show resolved
Hide resolved
| URI.create("http://" + request.headers().get(HOST) + request.getUri())); | ||
| } else { | ||
| return new URIDefaultDataAdapter(uri); | ||
| } |
There was a problem hiding this comment.
Maybe I'm picky here, but for curiosity sake I wonder if it's really worth using lambdas here when the difference is at most just one line of code. Without passing a lambda it would be almost the same from the reading complexity perspective.
final String uri = request.getUri();
final URI parsed = URIUtils.safeParse(uri);
if (parsed != null) {
if ((uri.getHost() == null || uri.getHost().equals(""))
&& request.headers().contains(HOST)) {
return URIDataAdapterBase.fromURI(
"http://" + request.headers().get(HOST) + request.getUri(),
URIDefaultDataAdapter::new);
}
return new URIDefaultDataAdapter(uri);
}
return new UnparseableURIDataAdapter(uri);There was a problem hiding this comment.
It would open opportunity to remove the mapper parameter completely and just use URIDefaultDataAdapter's constructor within URIDataAdapterBase.fromURI for simple use-cases.
| @@ -1493,6 +1497,23 @@ abstract class HttpServerTest<SERVER> extends WithHttpServer<SERVER> { | |||
| } | |||
| } | |||
|
|
|||
| def "test bad url not cause span marked as error"() { | |||
There was a problem hiding this comment.
Nice to have it covered with a test.
| return true; | ||
| } | ||
|
|
||
| public static URIDataAdapter fromURI(String uri, Function<URI, URIDataAdapter> mapper) { |
There was a problem hiding this comment.
As I mentioned before, I'm not completely sure if the mapper parameter is worth it. With or without it it's the same amount of boilerplate for the custom use-cases, and is more lightweight for simple cases.
There was a problem hiding this comment.
I agree with you. I leverage the mapping in the netty decorator, where the parsed uri is used, eventually, with host coming from the http headers to fix the url. So having a mapping function saves me some if and some other boilerplate to parse the URI. I may think about a better way to do it without having a mapper fuction and improve in a next PR
| @@ -156,4 +156,25 @@ class URINoRawDataAdapterTest extends URIDataAdapterTest { | |||
| return null | |||
| } | |||
| } | |||
|
|
|||
| static class UnparseableURIAdapterTest extends DDSpecification { | |||
amarziali
force-pushed
the
andrea.marziali/uri-tomcat
branch
from
c8c6d7c
to
ffb7916
Compare
What Does This Do
This PR is doing a couple of things:
Motivation
tldr; on recent tomcat versions backslashes are not allowed (it's configurable with
org.apache.catalina.connector.CoyoteAdapter.ALLOW_BACKSLASH=true) while before it was.This parsing error is captured because reported among the servlet response parameters. While we should log the stacktrace on the span, we do not should (unless configured) mark the span as error because we have a 4XX error
Additional Notes