feat(transactions): Add app_start_type migration #3124

philipphofmann · 2022-09-07T09:15:53Z

Adds the column app_start_type to transactions. For more context,
see getsentry/sentry#36927.

github-actions · 2022-09-07T09:16:54Z

This PR has a migration; here is the generated SQL

-- start migrations

-- forward migration transactions : 0017_transactions_add_app_start_type_column
Local operations:
ALTER TABLE transactions_local ADD COLUMN IF NOT EXISTS app_start_type LowCardinality(String) AFTER group_ids;


Dist operations:
ALTER TABLE transactions_dist ADD COLUMN IF NOT EXISTS app_start_type LowCardinality(String) AFTER group_ids;
-- end forward migration transactions : 0017_transactions_add_app_start_type_column




-- backward migration transactions : 0017_transactions_add_app_start_type_column
Local operations:
ALTER TABLE transactions_local DROP COLUMN IF EXISTS app_start_type;


Dist operations:
ALTER TABLE transactions_dist DROP COLUMN IF EXISTS app_start_type;
-- end backward migration transactions : 0017_transactions_add_app_start_type_column

philipphofmann · 2022-09-07T09:17:12Z

@evanh, are any further actions required to make that searchable in Discover?

evanh

Please separate the migration change from the other changes. Currently migrations in Snuba are run manually, so if this is merged in the current state inserts will start failing because the column doesn't exist on the table. The steps for adding a new column are:

Open a migration PR and a second PR for the reading/writing changes.
Merge the migration PR
Someone from SnS will manually run the migration
Merge the second PR.

are any further actions required to make that searchable in Discover?

The column also needs to be exposed in Sentry, via https://github.com/getsentry/sentry/blob/master/src/sentry/snuba/events.py

evanh · 2022-09-07T18:52:13Z

snuba/datasets/transactions_processor.py

@@ -248,6 +248,8 @@ def _process_contexts_and_user(
            if context in contexts:
                del contexts[context]

+        processed["app_start_type"] = contexts["app"]["start_type"]


Is this field guaranteed to always exist?

No, definitely not.

Ah, I just learned the difference between dict[key] and dict.get(key). I'm new to Python.

evanh · 2022-09-07T18:54:03Z

snuba/datasets/entities/discover.py

@@ -284,6 +284,7 @@ def attempt_map(
                ]
            ),
        ),
+        ("app_start_type", String(Modifiers(nullable=True))),


I would add/modify an E2E test in test_snql_api.py or test_snql_sdk_api.py that ensures this column can be read correctly.

philipphofmann · 2022-09-08T07:02:58Z

Thanks for the feedback, @evanh. I put everything together in one PR to be able to get feedback at one glance. I'm going to split the PR now.

codecov-commenter · 2022-09-08T07:53:49Z

Codecov Report

Base: 93.09% // Head: 93.09% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (3847067) compared to base (3d1084f).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff            @@
##           master    #3124    +/-   ##
========================================
  Coverage   93.09%   93.09%            
========================================
  Files         667      674     +7     
  Lines       30717    30908   +191     
========================================
+ Hits        28595    28775   +180     
- Misses       2122     2133    +11

Impacted Files	Coverage Δ
snuba/datasets/storages/transactions_common.py	`100.00% <ø> (ø)`
snuba/migrations/groups.py	`95.09% <ø> (ø)`
...ons/0017_transactions_add_app_start_type_column.py	`100.00% <100.00%> (ø)`
snuba/datasets/storages/storage_key.py	`96.77% <0.00%> (-3.23%)`	⬇️
snuba/attribution/log.py	`85.48% <0.00%> (-1.62%)`	⬇️
snuba/redis.py	`90.00% <0.00%> (ø)`
tests/fixtures.py	`100.00% <0.00%> (ø)`
snuba/web/views.py	`72.00% <0.00%> (ø)`
tests/test_snql_api.py	`100.00% <0.00%> (ø)`
snuba/datasets/entities/replays.py	`100.00% <0.00%> (ø)`
... and 128 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Adds reading and writing to the app_start_type . For more context, see getsentry/sentry#36927. This PR is based on the migration PR #3124.

…getsentry/sentry#36927. This PR is based on the migration PR #3124.

Related to getsentry/snuba#3124 and getsentry/snuba#3127. Fixes GH-36927

evanh

Please coordinate with the on-call in #discuss-eng-sns when you are ready to merge this, so that they can run the migration.

nikhars · 2022-09-12T20:48:05Z

snuba/datasets/storages/transactions_common.py

@@ -101,6 +101,7 @@
        ("transaction_source", String()),
        ("timestamp", DateTime(Modifiers(readonly=True))),
        ("group_ids", Array(UInt(64))),
+        ("app_start_type", String(Modifiers(readonly=True))),


Why do you add readonly=True modifier to this column? Is this column going to be materialized? If not, I would recommend that you remove readonly=True modifier

What exactly do you mean by materialized? I am OK with removing it.

nikhars · 2022-09-12T20:51:31Z

snuba/snuba_migrations/transactions/0017_transactions_add_app_start_type_column.py

+                table_name=table_name,
+                column=Column(
+                    "app_start_type",
+                    String(Modifiers(low_cardinality=True)),


Is app_start_type field user generated or machine generated? If it is machine generated and would have mostly a few values then having it as low_cardinality is ok. If it is user generated, what is the likelihood of having high cardinality of this value?

The SDKs create these values, and it's not exposed to the user. Currently, we have four different values. The likelihood of this having a high cardinality is very low. Maybe it increases to 10 or 20 at some point, but not more than 100 for sure.

…getsentry/sentry#36927. This PR is based on the migration PR #3124.

Adds reading and writing to the app_start_type . For more context, see getsentry/sentry#36927. This PR is based on the migration PR #3124. PR in Sentry to expose this via Discover. getsentry/sentry#38548.

Related to getsentry/snuba#3124 and getsentry/snuba#3127. Only merge after snuba PRs are merged. Fixes GH-36927

philipphofmann requested a review from evanh September 7, 2022 09:16

evanh reviewed Sep 7, 2022

View reviewed changes

feat(transactions): Add app_start_type

14d59e0

philipphofmann force-pushed the feat/app-start-type branch from 9e69fc3 to 14d59e0 Compare September 8, 2022 07:46

philipphofmann changed the title ~~feat(transactions): Add app_start_type~~ feat(transactions): Add app_start_type migration Sep 8, 2022

philipphofmann added a commit that referenced this pull request Sep 8, 2022

feat(transactions): app_start_type reading/writing

c770ac6

Adds reading and writing to the app_start_type . For more context, see getsentry/sentry#36927. This PR is based on the migration PR #3124.

philipphofmann added a commit that referenced this pull request Sep 8, 2022

Adds reading and writing to the app_start_type . For more context, see …

ec9136d

…getsentry/sentry#36927. This PR is based on the migration PR #3124.

philipphofmann mentioned this pull request Sep 8, 2022

feat(transactions): app_start_type reading/writing #3127

Merged

philipphofmann marked this pull request as ready for review September 8, 2022 09:18

philipphofmann requested a review from a team as a code owner September 8, 2022 09:18

philipphofmann added a commit to getsentry/sentry that referenced this pull request Sep 8, 2022

feat(mep): Add app start type to discover

f202467

Related to getsentry/snuba#3124 and getsentry/snuba#3127. Fixes GH-36927

philipphofmann mentioned this pull request Sep 8, 2022

feat(mep): Add app start type to discover getsentry/sentry#38548

Merged

philipphofmann requested a review from evanh September 9, 2022 06:17

philipphofmann self-assigned this Sep 9, 2022

philipphofmann mentioned this pull request Sep 9, 2022

Make app context start type searchable getsentry/sentry#36927

Closed

3 tasks

evanh approved these changes Sep 9, 2022

View reviewed changes

nikhars reviewed Sep 12, 2022

View reviewed changes

remove readonly

3847067

philipphofmann added a commit that referenced this pull request Sep 13, 2022

Adds reading and writing to the app_start_type . For more context, see …

d5f69a5

…getsentry/sentry#36927. This PR is based on the migration PR #3124.

onewland merged commit 2d5817d into master Sep 13, 2022

onewland deleted the feat/app-start-type branch September 13, 2022 14:34

philipphofmann mentioned this pull request Oct 4, 2022

feat(transactions): app_start_type reading/writing V2 #3209

Merged

philipphofmann added a commit to getsentry/sentry that referenced this pull request Oct 14, 2022

feat(mep): Add app start type to discover (#38548)

6973868

Related to getsentry/snuba#3124 and getsentry/snuba#3127. Only merge after snuba PRs are merged. Fixes GH-36927

barkbarkimashark pushed a commit to getsentry/sentry that referenced this pull request Oct 18, 2022

feat(mep): Add app start type to discover (#38548)

120609a

Related to getsentry/snuba#3124 and getsentry/snuba#3127. Only merge after snuba PRs are merged. Fixes GH-36927

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(transactions): Add app_start_type migration #3124

feat(transactions): Add app_start_type migration #3124

philipphofmann commented Sep 7, 2022 •

edited

Loading

github-actions bot commented Sep 7, 2022

philipphofmann commented Sep 7, 2022 •

edited

Loading

evanh left a comment

evanh Sep 7, 2022

philipphofmann Sep 8, 2022

philipphofmann Sep 8, 2022

evanh Sep 7, 2022

philipphofmann commented Sep 8, 2022

codecov-commenter commented Sep 8, 2022 •

edited

Loading

evanh left a comment

nikhars Sep 12, 2022

philipphofmann Sep 13, 2022

nikhars Sep 12, 2022

philipphofmann Sep 13, 2022

feat(transactions): Add app_start_type migration #3124

feat(transactions): Add app_start_type migration #3124

Conversation

philipphofmann commented Sep 7, 2022 • edited Loading

github-actions bot commented Sep 7, 2022

philipphofmann commented Sep 7, 2022 • edited Loading

evanh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philipphofmann commented Sep 8, 2022

codecov-commenter commented Sep 8, 2022 • edited Loading

Codecov Report

evanh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philipphofmann commented Sep 7, 2022 •

edited

Loading

philipphofmann commented Sep 7, 2022 •

edited

Loading

codecov-commenter commented Sep 8, 2022 •

edited

Loading