-
Notifications
You must be signed in to change notification settings - Fork 524
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.
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
Index page URL as ECS field #3857
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3857 +/- ##
==========================================
+ Coverage 79.73% 79.77% +0.03%
==========================================
Files 135 135
Lines 6158 6175 +17
==========================================
+ Hits 4910 4926 +16
- Misses 1248 1249 +1
|
💔 Build FailedExpand to view the summary
Build stats
Steps errorsExpand to view the steps failures
Log outputExpand to view the last 100 lines of log output
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just did a glance over, looks good so far.
I'm a little bit on the fence as to whether we should also add http.request.referrer
based on page.referer
, as suggested in #3827 (comment). Is that semantically correct? Might be worth checking with the RUM devs.
if fragment := url.Fragment; fragment != "" { | ||
out.Fragment = &fragment | ||
} | ||
host, port, err := net.SplitHostPort(url.Host) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aha, nice, we get this fixed as a bonus :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Just one question about package_tests, otherwise LGTM.
@@ -102,6 +102,7 @@ func transactionRequiredKeys() *tests.Set { | |||
"transaction.type", | |||
"transaction.context.request.method", | |||
"transaction.context.request.url", | |||
//"transaction.context.page.url", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's this about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...leftover 🙈
* Split Page URL into ECS parts and store it separately * Store page referer separately as ECS field * Update changelog
Motivation/summary
See:
Closes #3827
Checklist
I have considered changes for:
How to test these changes
Index a RUM event with something in
context.page
, and see it indexed as ECS fields inurl
andhttp.request.referrer
.Note: if agent sends
context.request.url
, that will be used for theurl
field, and not the page