Remove authority from URLs sent to Sentry#2366
Conversation
|
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 8fb596c | 355.54 ms | 438.55 ms | 83.01 ms |
| 46ba0df | 346.57 ms | 350.41 ms | 3.84 ms |
| 593148d | 368.38 ms | 413.20 ms | 44.82 ms |
| 15cc296 | 315.33 ms | 419.94 ms | 104.61 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 8fb596c | 1.73 MiB | 2.33 MiB | 620.61 KiB |
| 46ba0df | 1.73 MiB | 2.33 MiB | 621.15 KiB |
| 593148d | 1.73 MiB | 2.33 MiB | 621.15 KiB |
| 15cc296 | 1.73 MiB | 2.33 MiB | 620.61 KiB |
…lter request query params for okhttp
Codecov ReportBase: 80.12% // Head: 80.18% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #2366 +/- ##
============================================
+ Coverage 80.12% 80.18% +0.05%
- Complexity 3919 3943 +24
============================================
Files 322 323 +1
Lines 14796 14896 +100
Branches 1951 1966 +15
============================================
+ Hits 11856 11944 +88
- Misses 2171 2178 +7
- Partials 769 774 +5
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
adinauer
changed the title
cleptric
left a comment
cleptric
left a comment
There was a problem hiding this comment.
Just checked how the query and fragment are now set on the span/breadcrumb, LGTM!
markushi
left a comment
markushi
left a comment
There was a problem hiding this comment.
Nice one, I like the extensive tests!
| } | ||
|
|
||
| private static @NotNull String baseUrlOnly(final @NotNull String url) { | ||
| final int queryParamSeparatorIndex = url.indexOf("?"); |
There was a problem hiding this comment.
l: Can't you use java built-in URL class here instead?
I haven't tried it but
- Url.getPath should reflect url with query/ref
- Url.getRef should return the fragment
- Url.getQuery for the query params
There was a problem hiding this comment.
I guess you could reconstruct the base URL from parts retrieved from URL but imo that wouldn't be any better than the current code.
There was a problem hiding this comment.
Alright, then let's keep it this way. I just checked a few exotic URLs (like example.com/?/path/?query=param) and your solution provides better output than the URL api 👍
| return convertUrl(url); | ||
| } | ||
|
|
||
| public static @NotNull UrlDetails convertUrl(final @NotNull String url) { |
There was a problem hiding this comment.
l: I'm wondering if parse() or parseUrl() would suit better here
There was a problem hiding this comment.
No preference here. Can rename it if you want.
There was a problem hiding this comment.
Yeah I think it would fit better, but I'm definitely nit-picking here - if it's too much effort to refactor leave it as is 😅
| import java.util.regex.Pattern; | ||
| import org.jetbrains.annotations.NotNull; | ||
| import org.jetbrains.annotations.Nullable; | ||
|
|
||
| public final class UrlUtils { |
There was a problem hiding this comment.
I guess we could mark this as @ApiStatus.Internal right?
4 participants
📜 Description
Remove authority from URLs that are sent to Sentry, e.g. as span descriptions, breadcrumbs or requests.
💡 Motivation and Context
Fixes #2365
💚 How did you test it?
📝 Checklist
🔮 Next steps