Skip to content
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

Increase event_property threshold #2478

Merged
merged 2 commits into from Nov 4, 2021
Merged

Increase event_property threshold #2478

merged 2 commits into from Nov 4, 2021

Conversation

scholtzan
Copy link
Collaborator

I'm not sure if increasing this threshold has any implications?

@scholtzan scholtzan requested a review from wlach November 4, 2021 16:08
@@ -108,7 +108,7 @@ per_event_property AS (
USING
(category, event, event_property)
WHERE
event_property_value_index <= 1000
event_property_value_index <= 5000
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also open to setting a different threshold

Copy link
Contributor

Choose a reason for hiding this comment

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

I am slightly nervous about unintended consequences here of raising the limit by 5x, but I don't see why it would a problem apriori. It does seem reasonable to set a higher limit.

Copy link
Contributor

@wlach wlach left a comment

Choose a reason for hiding this comment

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

I'm not aware of any issues with increasing the threshold. However, I believe the right approach is to modify the templating here and regenerate the queries:

telemetry_derived:
name: Firefox
dataset: telemetry
source_table: telemetry_derived.deanonymized_events
start_date: 2020-01-01
max_property_values: 1000

@scholtzan
Copy link
Collaborator Author

Right, right

@scholtzan scholtzan requested a review from wlach November 4, 2021 17:50
@scholtzan scholtzan enabled auto-merge (rebase) November 4, 2021 17:50
Copy link
Contributor

@wlach wlach left a comment

Choose a reason for hiding this comment

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

r+ with the yaml changes to non-Firefox reverted

@@ -7,28 +7,28 @@ fenix_derived:
start_date: 2020-01-01
skipped_properties:
- time_ms
max_property_values: 1000
max_property_values: 5000
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep these values the same for everything other than Firefox desktop.

@@ -108,7 +108,7 @@ per_event_property AS (
USING
(category, event, event_property)
WHERE
event_property_value_index <= 1000
event_property_value_index <= 5000
Copy link
Contributor

Choose a reason for hiding this comment

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

I am slightly nervous about unintended consequences here of raising the limit by 5x, but I don't see why it would a problem apriori. It does seem reasonable to set a higher limit.

@scholtzan scholtzan merged commit 047135d into main Nov 4, 2021
@scholtzan scholtzan deleted the event-history-cap branch November 4, 2021 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants