Skip to content

feat: Use user_agent parsing for all events#10679

Merged
tonyo merged 5 commits intomasterfrom
feature/parse-contexts-for-all-events
Nov 21, 2018
Merged

feat: Use user_agent parsing for all events#10679
tonyo merged 5 commits intomasterfrom
feature/parse-contexts-for-all-events

Conversation

@HazAT
Copy link
Copy Markdown
Member

@HazAT HazAT commented Nov 20, 2018

Important

We need to add event-normalization.parse-user-agent-sample-rate to the database as a sentry option


This refactors the existing code from lang/javascript/plugin.py into context_normalization.py.
Additionally, this adds normalize_user_agent to EventManager normalize function with timing metrics.

We also added a dynamic setting to gradually increase this to 100% of events ingested.

Please note I've changed it so if there is already a browser, os or device context we no longer overwrite it, also for javascript events.

e.g: This is for a php event with correct user-agent header:

screenshot 2018-11-20 12 01 18
screenshot 2018-11-20 12 01 31

Before
screenshot 2018-11-20 12 02 29

To enable, merge: https://github.com/getsentry/getsentry/pull/2380

Fixes getsentry/sentry-php#700
Fixes #8555

@HazAT HazAT self-assigned this Nov 20, 2018
@HazAT HazAT changed the title feat: Use user_agent parsing for all events [WIP] feat: Use user_agent parsing for all events Nov 20, 2018
@HazAT HazAT requested review from mitsuhiko and tonyo November 20, 2018 12:19
@HazAT HazAT changed the title [WIP] feat: Use user_agent parsing for all events feat: Use user_agent parsing for all events Nov 20, 2018
Comment thread src/sentry/event_manager.py Outdated
fix_culprit(data)
if data.get('platform') == 'javascript':
inject_device_data(data)
normalize_user_agent(data)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this still necessary if we already perform normalize_user_agent in the event_manager?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it will be removed there once we validated that it works. For now we start with a small sample rate of events that get it as part of normalize since we don't know the performance impact of this.

Co-Authored-By: HazAT <daniel.griesser.86@gmail.com>
Copy link
Copy Markdown
Contributor

@mitsuhiko mitsuhiko left a comment

Choose a reason for hiding this comment

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

lgtm. I think we should try to experiment with this on 10% of events with op supervision.

@mitsuhiko mitsuhiko added Deploy: Monitored This PR requires a monitored deploy Deploy: Risky This is a particularly risky deploy labels Nov 20, 2018

try:
for key, value in headers:
if key != 'User-Agent':
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does the request object normalize header names to this casing?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yap and it's one of the things that's pretty annoying because it picks the wrong normalization. Modern HTTP requires lowercase :(

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good point, not sure, should I change this now?

@tonyo tonyo merged commit 1dbceb1 into master Nov 21, 2018
@tonyo tonyo deleted the feature/parse-contexts-for-all-events branch November 21, 2018 12:27
@github-actions github-actions Bot locked and limited conversation to collaborators Dec 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Deploy: Monitored This PR requires a monitored deploy Deploy: Risky This is a particularly risky deploy

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[2.0] User Agent - Header - Browser Browser not correctly recognized from the User-Agent header

5 participants