ref(MDC): Represent migrations in configuration #3071

evanh · 2022-08-17T22:05:35Z

This changes the migration groups to load automatically from configuration files
and populate the various global dictionaries we are currently using.

There is a new ConfigurationLoader which reads a migration group from a config
path. That path is detected automatically. The loaders are then added to the
global list of loaders and can be accessed in the same method as the hardcoded
loaders.

It is also showing one way to solve the "Enum" problem we have with different
components of Snuba.

lynnagara · 2022-08-18T22:48:20Z

A couple of initial thoughts/questions:

What is the goal of representing individual migrations as configuration at this point? Is it due to safety concerns (running custom code)? Usability concerns with the current system? Even if we want to support this at a point, I believe we will always have a need for migrations as code, and am worried about taking this flexibility away from users. We needed code migrations a few times already such as when we rebuilt the errors table and I don't think this will go away.
The migration groups mechanism seems fine to me - what is the purpose of having "kind" and "version" here though? Version at least seems like an entirely new concept, why do we need it?

evanh · 2022-08-19T14:56:43Z

The general consensus I'm gathering on this is: Don't make individual migrations part of the config, but making migration groups part of the configuration makes sense. I'm going to move forward with that.

what is the purpose of having "kind" and "version" here though?

These are standard fields we are adding to all the config files, so they are self describing and we can change the schemas a little easier in the future.

lynnagara · 2022-08-19T17:50:38Z

Separate to the representation, I think there will be a few other questions we will have to answer as part of this. How will the initial system migrations (which are mandatory) be fit into this system. How will dependencies between migration groups be resolved? We currently use the OPTIONAL_MIGRATION_GROUPS mechanism for experimental product features - what is the future of this?

fpacifici · 2022-08-19T18:53:29Z

Don't make individual migrations part of the config, but making migration groups part of the configuration makes sense. I'm going to move forward with that.

Should we make migration groups auto-discoverable ?
We did not do that at the beginning because there were corner cases that were not trivial to manage the sequence and potential merge conflicts in the same group.
Though in a world where we want to reduce the friction and reduce the places the product teams need to touch it may be a direction to reconsider.
We could do something like this:

only mention a new group in settings when they are created, or even in the dataset configuration.
auto-discover the migration files in the group directory
expect the migration files to be in sequence (keep the number at the beginning). It is something relatively easy to automatically test.

fpacifici · 2022-08-19T18:05:30Z

snuba/datasets/configuration/generic_metrics/migrations/0001_sets_aggregate_table.yaml

+  - add_column
+    storage_set: generic_metrics_sets
+    table_name: generic_metric_sets_local
+    column:
+      name: _indexed_tags_hash
+      type: Array
+      args: { type: UInt, arg: 64 }
+      modifiers: { materialized: "arrayMap((k, v) -> cityHash64(concat(toString(k), '=', toString(v))), tags.key, tags.indexed_value)" }
+    after: null
+
+  - add_column
+    storage_set: generic_metrics_sets
+    table_name: generic_metric_sets_local
+    column:
+      name: _raw_tags_hash
+      type: Array
+      args: { type: UInt, arg: 64 }
+      modifiers: { materialized: "arrayMap((k, v) -> cityHash64(concat(toString(k), '=', v)), tags.key, tags.raw_value)" }
+    after: null


@onewland, fyi, these columns should be created on the distributed table as well. In fact they are missing in production.

github-actions · 2022-08-30T20:34:35Z

This PR has a migration; here is the generated SQL

CONFIG {'version': 'v1', 'kind': 'migration_group', 'name': 'generic_metrics', 'optional': True, 'migrations': ['0001_sets_aggregate_table', '0002_sets_raw_table', '0003_sets_mv', '0004_sets_raw_add_granularities', '0005_sets_replace_mv', '0006_sets_raw_add_granularities_dist_table', '0007_distributions_aggregate_table', '0008_distributions_raw_table', '0009_distributions_mv']}

snuba/migrations/groups.py

codecov-commenter · 2022-08-30T20:44:31Z

Codecov Report

Base: 92.84% // Head: 93.03% // Increases project coverage by +0.19% 🎉

Coverage data is based on head (550d0dd) compared to base (3bc37db).
Patch coverage: 96.92% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3071      +/-   ##
==========================================
+ Coverage   92.84%   93.03%   +0.19%     
==========================================
  Files         673      676       +3     
  Lines       30893    30914      +21     
==========================================
+ Hits        28683    28762      +79     
+ Misses       2210     2152      -58

Impacted Files	Coverage Δ
snuba/cli/migrations.py	`40.95% <0.00%> (ø)`
snuba/migrations/runner.py	`91.66% <50.00%> (ø)`
...ba/datasets/configuration/migration_json_schema.py	`100.00% <100.00%> (ø)`
snuba/datasets/configuration/migration_parser.py	`100.00% <100.00%> (ø)`
snuba/migrations/groups.py	`95.09% <100.00%> (ø)`
snuba/settings/__init__.py	`94.48% <100.00%> (+0.04%)`	⬆️
...ts/datasets/configuration/test_migration_groups.py	`100.00% <100.00%> (ø)`
tests/migrations/test_groups.py	`100.00% <100.00%> (ø)`
tests/migrations/test_runner.py	`99.15% <100.00%> (ø)`
tests/migrations/test_runner_individual.py	`91.20% <100.00%> (-2.20%)`	⬇️
... and 16 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.

StorageKey enum replaced with a class. Based on a working solution for MigrationGroup here: #3071

This changes the migration groups to load automatically from configuration files and populate the various global dictionaries we are currently using. There is a new `ConfigurationLoader` which reads a migration group from a config path. That path is detected automatically. The loaders are then added to the global list of loaders and can be accessed in the same method as the hardcoded loaders. It is also showing one way to solve the "Enum" problem we have with different components of Snuba.

snuba/datasets/configuration/migration_parser.py

snuba/migrations/groups.py

volokluev · 2022-09-07T20:33:18Z

tests/datasets/configuration/test_migration_groups.py

+    assert generic_metrics_loader.get_migrations() == [
+        "0001_sets_aggregate_table",
+        "0002_sets_raw_table",
+        "0003_sets_mv",
+        "0004_sets_raw_add_granularities",
+        "0005_sets_replace_mv",
+        "0006_sets_raw_add_granularities_dist_table",
+        "0007_distributions_aggregate_table",
+        "0008_distributions_raw_table",
+        "0009_distributions_mv",
+    ]
+
+    m = generic_metrics_loader.load_migration("0005_sets_replace_mv")
+    assert isinstance(m, Migration)


since migrations are likely to be added, I would just iterate over them and load_migration in order to make sure that they are all indeed migrations. Maybe assert that the len(migrations) > 5 or something.
This way when another one is added, you won't need to change the test

I'm going to leave this because I think we should have a test like this. I do agree though that it will be annoying for generic metrics to have to update this test as well. I think in the future we maybe will want a dummy dataset we can do these kinds of tests on.

volokluev

all my comments are not required for merging

lynnagara · 2022-09-07T21:39:02Z

snuba/migrations/groups.py

@@ -73,6 +50,27 @@ def load_migration(self, migration_id: str) -> Migration:
            raise MigrationDoesNotExist("Invalid migration ID")


+class ConfigurationLoader(DirectoryLoader):


I think this should inherit from the base GroupLoader not DirectoryLoader which supposed to be a completely separate implementation

I did this because otherwise I'd have to copy the load_migration function over.

lynnagara · 2022-09-07T21:47:30Z

Please also update MIGRATIONS.md

lynnagara · 2022-09-09T17:48:02Z

Reposting an earlier comment since it was marked resolved without a response. I'm still interested in your thoughts on this. If you have other reasons for keeping this interface I'll be happy to hear them but the only reason stated is to avoid changing a few spots in the codebase and that just doesn't seem right to me.

I don't think this enum like interface is the right one anymore since Migration groups can be defined on the fly. It made sense previously because all of the members were defined in code. The rest of the codebase never references MigrationGroup (only a handful of places within the migrations module) so there are not that many places to change.

This deletes the Enum and removes the need for the metaclass.

It's not clear that migrations belong in configuration. This was already an issue from the previous work, since the migrations themselves are still part of the code. There is now a separate effort to figure out where migrations should live, so reverting these changes in the mean time to avoid having things in a half finished state. Before state: The generic metrics MigrationGroup was defined in configuration, and the groups code was loading that configuration. The MigrationGroup was a string to allow for groups being defined outside of Python code. After state: All MigrationGroups are defined in the python file, and MigrationGroup has been changed back to an Enum. This reverts commit ae89a81

* Revert "ref(MDC): Represent migrations in configuration (#3071)" It's not clear that migrations belong in configuration. This was already an issue from the previous work, since the migrations themselves are still part of the code. There is now a separate effort to figure out where migrations should live, so reverting these changes in the mean time to avoid having things in a half finished state. Before state: The generic metrics MigrationGroup was defined in configuration, and the groups code was loading that configuration. The MigrationGroup was a string to allow for groups being defined outside of Python code. After state: All MigrationGroups are defined in the python file, and MigrationGroup has been changed back to an Enum. This reverts commit ae89a81

evanh added the do not merge label Aug 17, 2022

fpacifici reviewed Aug 19, 2022

View reviewed changes

evanh force-pushed the evanh/feat/migrations-as-config branch from 564b206 to d1e0ee9 Compare August 30, 2022 20:33

evanh marked this pull request as ready for review August 30, 2022 20:34

evanh requested a review from a team as a code owner August 30, 2022 20:34

volokluev reviewed Aug 30, 2022

View reviewed changes

snuba/migrations/groups.py Outdated Show resolved Hide resolved

rahul-kumar-saini mentioned this pull request Aug 30, 2022

ref(MDC): StorageKey enum to class #3096

Merged

rahul-kumar-saini added a commit that referenced this pull request Sep 1, 2022

ref(MDC): StorageKey enum to class (#3096)

ef6260d

StorageKey enum replaced with a class. Based on a working solution for MigrationGroup here: #3071

evanh removed the do not merge label Sep 1, 2022

evanh force-pushed the evanh/feat/migrations-as-config branch from a2055e0 to 82cb0a6 Compare September 1, 2022 20:32

evanh force-pushed the evanh/feat/migrations-as-config branch from 82cb0a6 to d990750 Compare September 1, 2022 20:33

volokluev reviewed Sep 7, 2022

View reviewed changes

volokluev approved these changes Sep 7, 2022

View reviewed changes

lynnagara reviewed Sep 7, 2022

View reviewed changes

evanh added 2 commits September 8, 2022 10:31

some fixes

bc9f7c6

merge

c069ed5

merge

6c354f6

evanh mentioned this pull request Sep 12, 2022

evanh/feat/migration group configs #3133

Closed

Replace MigrationGroup with a string

550d0dd

This deletes the Enum and removes the need for the metaclass.

evanh merged commit ae89a81 into master Sep 13, 2022

evanh deleted the evanh/feat/migrations-as-config branch September 13, 2022 17:41

evanh mentioned this pull request Oct 25, 2022

feat(TDC): Migrate transactions migrate group to configuration #3290

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref(MDC): Represent migrations in configuration #3071

ref(MDC): Represent migrations in configuration #3071

evanh commented Aug 17, 2022 •

edited

Loading

lynnagara commented Aug 18, 2022

evanh commented Aug 19, 2022

lynnagara commented Aug 19, 2022

fpacifici commented Aug 19, 2022

fpacifici Aug 19, 2022

github-actions bot commented Aug 30, 2022

codecov-commenter commented Aug 30, 2022 •

edited

Loading

volokluev Sep 7, 2022

evanh Sep 8, 2022

volokluev left a comment

lynnagara Sep 7, 2022

evanh Sep 8, 2022

lynnagara commented Sep 7, 2022

lynnagara commented Sep 9, 2022 •

edited

Loading

		@@ -73,6 +50,27 @@ def load_migration(self, migration_id: str) -> Migration:
		raise MigrationDoesNotExist("Invalid migration ID")


		class ConfigurationLoader(DirectoryLoader):

ref(MDC): Represent migrations in configuration #3071

ref(MDC): Represent migrations in configuration #3071

Conversation

evanh commented Aug 17, 2022 • edited Loading

lynnagara commented Aug 18, 2022

evanh commented Aug 19, 2022

lynnagara commented Aug 19, 2022

fpacifici commented Aug 19, 2022

fpacifici Aug 19, 2022

Choose a reason for hiding this comment

github-actions bot commented Aug 30, 2022

codecov-commenter commented Aug 30, 2022 • edited Loading

Codecov Report

volokluev Sep 7, 2022

Choose a reason for hiding this comment

evanh Sep 8, 2022

Choose a reason for hiding this comment

volokluev left a comment

Choose a reason for hiding this comment

lynnagara Sep 7, 2022

Choose a reason for hiding this comment

evanh Sep 8, 2022

Choose a reason for hiding this comment

lynnagara commented Sep 7, 2022

lynnagara commented Sep 9, 2022 • edited Loading

evanh commented Aug 17, 2022 •

edited

Loading

codecov-commenter commented Aug 30, 2022 •

edited

Loading

lynnagara commented Sep 9, 2022 •

edited

Loading