Skip to content

Safely parse URL and respect http server status code error rule#5128

Merged
amarziali merged 1 commit into
masterfrom
andrea.marziali/uri-tomcat
May 2, 2023
Merged

Safely parse URL and respect http server status code error rule#5128
amarziali merged 1 commit into
masterfrom
andrea.marziali/uri-tomcat

Conversation

@amarziali

Copy link
Copy Markdown
Contributor

What Does This Do

This PR is doing a couple of things:

  • Safely parse the URL. If the URL is un parsable, when possible, tag the span with the raw uri (and not the parsed one since it will be null or broken)
  • For tomcat, make sure that when an exception is captured, the span is marked as errored only if the http response code matches the related rule

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

@amarziali
amarziali requested a review from a team as a code owner April 27, 2023 11:37
@amarziali
amarziali force-pushed the andrea.marziali/uri-tomcat branch from 6b43747 to 1df795e Compare April 27, 2023 11:44
@amarziali amarziali added the inst: others All other instrumentations label Apr 27, 2023
@pr-commenter

pr-commenter Bot commented Apr 27, 2023

Copy link
Copy Markdown

Benchmarks

Parameters

Baseline Candidate
commit 1.13.0-SNAPSHOT~6dd932c714 1.13.0-SNAPSHOT~ffb79165e5
config baseline candidate
See matching parameters
Baseline Candidate
module Agent Agent
parent None None

Summary

Found 0 performance improvements and 0 performance regressions! Performance is the same for 22 cases.

See unchanged results
scenario Δ mean execution_time
scenario:Startup-base-Agent same
scenario:Startup-base-Agent.start same
scenario:Startup-base-BytebuddyAgent same
scenario:Startup-base-GlobalTracer same
scenario:Startup-base-AppSec same
scenario:Startup-base-Remote Config same
scenario:Startup-base-Telemetry unsure
[+0.057ms; +0.323ms] or [+0.945%; +5.390%]
scenario:Startup-iast-Agent same
scenario:Startup-iast-Agent.start same
scenario:Startup-iast-BytebuddyAgent same
scenario:Startup-iast-GlobalTracer same
scenario:Startup-iast-AppSec unsure
[+0.267ms; +2.606ms] or [+0.263%; +2.569%]
scenario:Startup-iast-IAST unsure
[+0.061ms; +0.633ms] or [+0.546%; +5.656%]
scenario:Startup-iast-Remote Config same
scenario:Startup-iast-Telemetry unsure
[+0.005ms; +0.332ms] or [+0.080%; +5.528%]
scenario:Startup-waf-Agent same
scenario:Startup-waf-Agent.start same
scenario:Startup-waf-BytebuddyAgent same
scenario:Startup-waf-GlobalTracer same
scenario:Startup-waf-AppSec same
scenario:Startup-waf-Remote Config same
scenario:Startup-waf-Telemetry same

@amarziali
amarziali force-pushed the andrea.marziali/uri-tomcat branch 6 times, most recently from 46ffec0 to c8c6d7c Compare April 27, 2023 14:01

@ygree ygree Apr 28, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would open opportunity to remove the mapper parameter completely and just use URIDefaultDataAdapter's constructor within URIDataAdapterBase.fromURI for simple use-cases.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to have it covered with a test.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

@ygree ygree left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Just not sure about worthiness of the mapper parameter that takes a lambda.

@amarziali
amarziali force-pushed the andrea.marziali/uri-tomcat branch from c8c6d7c to ffb7916 Compare May 2, 2023 10:16
@amarziali
amarziali merged commit 467e186 into master May 2, 2023
@amarziali
amarziali deleted the andrea.marziali/uri-tomcat branch May 2, 2023 13:29
@github-actions github-actions Bot added this to the 1.13.0 milestone May 2, 2023
smola added a commit that referenced this pull request May 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

inst: others All other instrumentations

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants