Set Event.http.request.url on a best effort basis #99
Conversation
…l.getFullUrl() checks for absolute url now
findingrish
requested review from
a team,
GurtejSohi,
buchi-busireddy,
kotharironak,
skjindal93 and
tim-mwangi
| url.ifPresent(requestBuilder::setUrl); | ||
| } | ||
|
|
||
| static boolean isAbsoluteUrl(String urlStr) { |
There was a problem hiding this comment.
I believe it's the same method defined in EnrichedSpanUtils above.
There was a problem hiding this comment.
Yes it is, since span-normalizer doesn't depend on enriched-span-constants or vice-versa, I just duplicated the code.
| return Optional.empty(); | ||
| } | ||
|
|
||
| private static boolean isAbsoluteUrl(String urlStr) { |
There was a problem hiding this comment.
Who is using the private function?
There was a problem hiding this comment.
This method getFullHttpUrl
|
|
||
| private static boolean isAbsoluteUrl(String urlStr) { | ||
| try { | ||
| URL url = new URL(new URL("http://hypertrace.org"), urlStr); |
There was a problem hiding this comment.
Why do we need this http://hypertrace.org?
There was a problem hiding this comment.
Any other url will be fine too. In case when input url is relative, this provided url makes up for the scheme, authority etc.
| if (event.getHttp() != null | ||
| && event.getHttp().getRequest() != null | ||
| && event.getHttp().getRequest().getUrl() != null | ||
| && isAbsoluteUrl(event.getHttp().getRequest().getUrl())) { |
There was a problem hiding this comment.
when the URL isn't full URL, do you want to rather check if you can construct full URL back from scheme, authority, path and query params? That's what I had in mind.
There was a problem hiding this comment.
Oh.. if you're making sure this URL is always full URL, we don't even need this method I think.
There was a problem hiding this comment.
We would need it.
Earlier, event.getHttp().getRequest().getUrl() was null in case of relative url. Now since it is being populated in a best effort basis, so it maybe full url or not.
However the consumer of this method EnrichedSpanUtils#getFullHttpUrl would except a full url to be returned or null.
To ensure that behaviour a check has been added to this method.
However I think we can go with this pr first #104 for now. It is a subset of the changes made in this pr.
...-generator/src/main/java/org/hypertrace/viewgenerator/generators/SpanEventViewGenerator.java
Outdated
Show resolved
Hide resolved
...er/src/main/java/org/hypertrace/core/spannormalizer/fieldgenerators/HttpFieldsGenerator.java
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## main #99 +/- ##
============================================
- Coverage 68.63% 68.56% -0.07%
- Complexity 819 822 +3
============================================
Files 84 84
Lines 3475 3487 +12
Branches 367 370 +3
============================================
+ Hits 2385 2391 +6
- Misses 946 950 +4
- Partials 144 146 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Can we close the PR, if it's not needed? I see it's already been taken care of here https://github.com/hypertrace/hypertrace-ingester/pull/108/files. Correct me, if I am wrong |
Yes, closing it. |
Problem: Currently we set
Event.http.request.urlas null in case when http url is relative. This causes problems in some of the consumers ofEvent.http.request.urlThere are 3 changes in this pr:
Event.http.request.urlwas originally being set using url attributes, although in the case of relative url it was being set as null. That has been changed with this pr. Now we will pass it as it is and let the consumer decide.Secondly, most of the consumer of full url, use EnrichedSpanUtil#getFullUrl (which internally fetches url from
Event.http.request.url), so this method has been updated with a check for absolute url.Third, SpanEventViewGenerator has been updated to consume whatever is being set in
Event.http.request.urlinstead of full urlThe other option is,
Event.http.request.urlas null for relative url