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
Round dimension type values on payload #1196
Round dimension type values on payload #1196
Conversation
BundleMonFiles added (6)
Total files change +94.92KB 0% Final result: ✅ View report in BundleMon website ➡️ |
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.
LGTM, just one nitpick with my preference to round the values before joining them into a string.
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.
No real problems, LGTM!
resolution: screen.width + 'x' + screen.height, | ||
viewport: floorDimensionFields(detectViewport()), | ||
documentSize: floorDimensionFields(detectDocumentSize()), | ||
resolution: floorDimensionFields(detectScreenResolution()), |
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.
Have we confirmed when the fractional values actually occur (which useragent/circumstances)? It would be good to have that concrete before we throw away data... and before someone inevitably asks why it's wrong for that random user agent.
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.
throw away data
In what case will this occur ? Or you mean, not allowing the fractional part to get through ?
The change in behaviour from this PR is only that fractional values will be rounded down to the closest int for these attributes.
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.
Yes, just that we're discarding that fractional info. Its existence means something (bot?), I just don't know what, and we're throwing that data point away by rounding it. 😛
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.
Thought about it a bit, let me provide some of my thoughts:
- If the
innerWidth
or any other value is fractional probably means there is some user-agent bug or unexpected behaviour. It does not automatically signal bot traffic though. - Does a condition like a fractional value on width/height make it a bad event that needs to be manually reviewed without having a clear response if it is indeed bad ? That's where I am stuck in the logic.
- We cannot change the type of the parameter to accept fractional values as it is part of the atomic table and will probably be a breaking change.
- What we can is introduce a
browser_context
model change (not sure we even have it modeled yet) where we allow for fractional values to be pushed in and analyzed normally without ending in the bad queue.
WDYT ?
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.
To be clear, I have no problem with this change and agree with it - we just need to know when/why the fractional value occurs if possible, so it's explainable why the captured value is different from the real value.
Like if atomic.events had a maxLength on useragent of like 120 and we made the trackers .slice(0,120) the value so it was always good, we would probably want to document some of the known cases of it being over 120 characters to at least understand why the values are partial sometimes, right?
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.
Yeap, I will add a task for that so it will remain on our radar! I totally agree
8660208
to
b4be53a
Compare
Round dimension type values.
Notes:
addPayloadPair
more lenient aspayloadBuilder.add
close #1195