Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
github-actions
Bot
added
api
Docker builds report
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5823 +/- ##
=======================================
Coverage 97.82% 97.82%
=======================================
Files 1258 1258
Lines 44729 44745 +16
=======================================
+ Hits 43755 43771 +16
Misses 974 974 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
emyller
left a comment
emyller
left a comment
There was a problem hiding this comment.
Looks good in general! Let's choose a safer option to parse UAs if you agree.
| elif label == "user_agent": | ||
| parsed_ua_string: str = parse_ua(value) | ||
| # Assume UA strings categorised as "Other" to represent server-side SDKs. | ||
| # Skip browser SDKs that don't send the special header. | ||
| if parsed_ua_string.split(" - ")[0] != "Other": | ||
| continue |
There was a problem hiding this comment.
I'm not sure what's happening here, despite the comment, sorry. I assume it's domain-specific to fastuaparser?
The alternative uap-parser (mentioned in the other thread) might offer a cleaner API, e.g.
| elif label == "user_agent": | |
| parsed_ua_string: str = parse_ua(value) | |
| # Assume UA strings categorised as "Other" to represent server-side SDKs. | |
| # Skip browser SDKs that don't send the special header. | |
| if parsed_ua_string.split(" - ")[0] != "Other": | |
| continue | |
| # Assume unrecognized User Agents to represent server-side SDKs | |
| elif label == "user_agent" and not parse_user_agent(value): | |
| continue |
There was a problem hiding this comment.
I'm not sure what's happening here, despite the comment, sorry.
Let me know how can we improve the comment.
The alternative uap-parser (mentioned in the other thread) might offer a cleaner API, e.g.
Sure, but see considerations above.
There was a problem hiding this comment.
Let me know how can we improve the comment.
The suggested snippet includes a suggestion of comment, though its readability largely depends on not having to explain how fastuaparser works. If you're adamant in keeping fastuaparser considering my last question, I think a slightly longer comment here could help. Also, sorry, my suggestion was incomplete in respect to the "skip browser SDKs" bit.
| elif label == "user_agent": | |
| parsed_ua_string: str = parse_ua(value) | |
| # Assume UA strings categorised as "Other" to represent server-side SDKs. | |
| # Skip browser SDKs that don't send the special header. | |
| if parsed_ua_string.split(" - ")[0] != "Other": | |
| continue | |
| elif label == "user_agent": | |
| # fastuaparser classifies unrecognized UAs as "Other" — assume these to | |
| # represent server-side SDKs. | |
| parsed_ua_string: str = parse_ua(value) | |
| is_server_side_sdk = parsed_ua_string.startswith("Other - ") | |
| # Skip browser SDKs that don't send the special header | |
| if not is_server_side_sdk: | |
| continue |
There was a problem hiding this comment.
Sorry, applying the suggestion broke the logic. Fixed in 0b7e0d8, but need a re-approval now.
Co-authored-by: Evandro Myller <22429+emyller@users.noreply.github.com>
for more information, see https://pre-commit.ci
github-actions
Bot
added
feature
github-actions
Bot
added
feature
2 participants
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature!Changes
Closes #5735.
This adds
User-Agentprocessing ahead of labelling the usage and feature evaluation data. Browser UA strings are stripped, the rest are considered server-side SDKs, and a newFlagsmith-SDK-User-Agentheader is expected to denote a browser SDK version.How did you test this code?
Updated existing view unit test to reflect the new behaviour.