feat(rum-core): add network info for all transactions#752
feat(rum-core): add network info for all transactions#752vigneshshanmugam wants to merge 3 commits intoelastic:mainfrom
Conversation
| function getNetworkInformation() { | ||
| const connection = navigator && navigator.connection | ||
|
|
||
| if (!(connection && typeof connection === 'object')) { |
There was a problem hiding this comment.
It's better to not negate the condition and just wrap the following return inside if.
| ...userContext | ||
| }) | ||
|
|
||
| expect(pageloadTr.context.page.netinfo).toEqualOrUndefined({ |
There was a problem hiding this comment.
IMO, using toEqualOrUndefined is not a good idea, instead we should detect if the feature that is required for this to work exist and we should add explicit checks for the outcomes. With this matcher we don't know whether the result was undefined because of a bug or because the api is not supported.
There was a problem hiding this comment.
If i get it right, do you want to test it only for supported browsers? Like we do explicit check like
if (connection) {
expect(measuredInfo).toBe(actualInfo)?
}I was trying to express this using the matcher, May be i missed something. We can also do explicit checks in tests if its confusing.
There was a problem hiding this comment.
correct! also check the else condition, i.e:
if (connection) {
expect(measuredInfo).toBe(actualInfo)
} else {
expect(measuredInfo).toBe(undefined)
}There was a problem hiding this comment.
Yeah sounds good 👍
💔 Build Failed
Expand to view the summary
Build stats
Test stats 🧪
Test errorsExpand to view the tests failures
Steps errorsExpand to view the steps failures
Log outputExpand to view the last 100 lines of log output
|
|
On hold till the spec is finalised on the APM Server- More details elastic/apm-server#3918 (comment) |
2 participants
page-load,route-changeand other auto instrumented transactions would include this information by default under transaction page metadata.Server schema spec - elastic/apm-server#3657