Skip to content

Set Event.http.request.url on a best effort basis - 2 #104

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Jan 11, 2021
Merged

Set Event.http.request.url on a best effort basis - 2 #104

merged 2 commits into from
Jan 11, 2021

Conversation

@findingrish
Copy link
Contributor

@findingrish findingrish commented Jan 8, 2021

Option 2 of #99

The changes include:

  1. Update HttpFieldsGenerator#maybeSetHttpUrlForOtelFormat to overwrite relative url
  2. Update the logic to fetch http url in SpanEventViewGenerator

1. Update HttpFieldsGenerator#maybeSetHttpUrlForOtelFormat to overwri…
ac63dd0 
…te relative url

2. Update the logic to fetch http url in SpanEventViewGenerator
@findingrish findingrish requested a review from a team January 8, 2021 11:25
@codecov
Copy link

codecov bot commented Jan 8, 2021 •
edited
Loading

Codecov Report

Merging #104 (ece101e) into main (6b774bf) will decrease coverage by 0.00%.
The diff coverage is 75.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #104      +/-   ##
============================================
- Coverage     68.63%   68.62%   -0.01%     
- Complexity      819      821       +2     
============================================
  Files            84       84              
  Lines          3475     3481       +6     
  Branches        367      367              
============================================
+ Hits           2385     2389       +4     
- Misses          946      948       +2     
  Partials        144      144
Flag Coverage Δ Complexity Δ
unit 68.62% <75.00%> (-0.01%) 0.00 <4.00> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
...ormalizer/fieldgenerators/HttpFieldsGenerator.java 93.75% <60.00%> (-0.53%) 105.00 <4.00> (+2.00) ⬇️
...ewgenerator/generators/SpanEventViewGenerator.java 9.32% <100.00%> (+1.56%) 5.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b774bf...ece101e. Read the comment docs.

@github-actions

This comment has been minimized.

Sign in to view
@findingrish findingrish mentioned this pull request Jan 8, 2021
Closed
@findingrish findingrish changed the title Set Event.http.request.url on a best effort basis Set Event.http.request.url on a best effort basis - 2 Jan 8, 2021
tim-mwangi
tim-mwangi reviewed Jan 8, 2021
View reviewed changes
...er/src/main/java/org/hypertrace/core/spannormalizer/fieldgenerators/HttpFieldsGenerator.java
try {
URL url = getNormalizedUrl(urlStr);
return url.toString().equals(urlStr);
} catch (MalformedURLException e) {
Copy link
Contributor

@tim-mwangi tim-mwangi Jan 8, 2021

Choose a reason for hiding this comment

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

Can we add unit tests for these negative scenarios?(If possible)

Copy link
Contributor Author

@findingrish findingrish Jan 11, 2021

Choose a reason for hiding this comment

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

The java implementation for URL conversion seems to be able to parse all the strings! I will update the test, when I find one.

@buchi-busireddy
Copy link
Contributor

buchi-busireddy commented Jan 11, 2021 •
edited
Loading

@rish691 So, if I understand it right, you're moving to a model where:

  • span.http.request.url will be set only with the full URL
  • if the full URL is missing, the consumer can decide what to do. View-gen is using path because it's for display in the UI.
    Can you confirm?

@findingrish
Copy link
Contributor Author

findingrish commented Jan 11, 2021 •
edited
Loading

@rish691 So, if I understand it right, you're moving to a model where:

  • span.http.request.url will be set only with the full URL

So, presently also span.http.request.url is set only in the case of full URL. The change in this PR is to just the increase the chances of it being set to a full url.

  • if the full URL is missing, the consumer can decide what to do. View-gen is using path because it's for display in the UI.

If we still see problems downstream which can not be solved with changes there itself, we can revisit the logic of setting span.http.request.url as null.

@github-actions

This comment has been minimized.

Sign in to view
@findingrish findingrish merged commit c529c39 into main Jan 11, 2021
@findingrish findingrish deleted the populate-http-url-option-2 branch January 11, 2021 07:37
@github-actions
Copy link

github-actions bot commented Jan 11, 2021

Unit Test Results

  47 files  ±0    47 suites  ±0   53s ⏱️ +8s
241 tests +5  241 ✔️ +5  0 💤 ±0  0 ❌ ±0 

Results for commit c529c39. ± Comparison against base commit 6b774bf.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tim-mwangi tim-mwangi tim-mwangi left review comments

@GurtejSohi GurtejSohi Awaiting requested review from GurtejSohi

@kotharironak kotharironak Awaiting requested review from kotharironak

@skjindal93 skjindal93 Awaiting requested review from skjindal93

+1 more reviewer

@buchi-busireddy buchi-busireddy buchi-busireddy approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@findingrish @buchi-busireddy @tim-mwangi