Skip to content

Commit 80a2310

Browse files
jcscottiiijcscottiii
authored
Introduce generic migrator for feature key renames & add use it for dev signals (#1787)
Introduces a new generic `Migrator` in `lib/gcpspanner/spanneradapters` to handle data migration when a web feature key is renamed or moved. This centralizes the logic for checking conflicts and applying updates when a feature ID changes in an upstream data source. The previous implementation had duplicated and specific logic within each data consumer (`ChromiumHistogramEnumConsumer` and `DeveloperSignalsConsumer`) to handle these migrations. This new approach provides a single, reusable, and testable component for this task. This commit refactors the following consumers to use the new `Migrator`: - `ChromiumHistogramEnumConsumer`: The existing migration logic is replaced with the new migrator. - `DeveloperSignalsConsumer`: Migration logic is added to handle moved features before syncing data. Comprehensive unit tests for the new `Migrator` are included, covering successful migrations, conflicts, and logging. The tests for the affected consumers have also been updated to reflect the new implementation.
1 parent 81d813f commit 80a2310

File tree

6 files changed

+455
-79
lines changed

6 files changed

+455
-79
lines changed

‎lib/gcpspanner/spanneradapters/chromium_historgram_enum_consumer.go‎

Lines changed: 15 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -52,8 +52,6 @@ type ChromiumHistogramEnumsClient interface {
5252
// Used by GCP Log-based metrics to extract the data about mismatch mappings.
5353
const logMissingFeatureIDMetricMsg = "unable to find feature ID. skipping mapping"
5454

55-
var ErrConflictMigratingFeatureKey = errors.New("conflict migrating feature key")
56-
5755
func (c *ChromiumHistogramEnumConsumer) GetAllMovedWebFeatures(
5856
ctx context.Context) (map[string]web_platform_dx__web_features.FeatureMovedData, error) {
5957
movedFeatures, err := c.client.GetAllMovedWebFeatures(ctx)
@@ -80,7 +78,8 @@ func (c *ChromiumHistogramEnumConsumer) SaveHistogramEnums(
8078
return err
8179
}
8280

83-
err = migrateMovedFeatures(ctx, histogramsToEnumMap, histogramsToAllFeatureKeySet, movedFeatures)
81+
err = migrateMovedFeaturesForChromiumHistograms(
82+
ctx, histogramsToEnumMap, histogramsToAllFeatureKeySet, movedFeatures)
8483
if err != nil {
8584
return err
8685
}
@@ -162,34 +161,25 @@ func buildHistogramMaps(
162161
return histogramsToEnumMap, histogramsToAllFeatureKeySet
163162
}
164163

165-
func migrateMovedFeatures(
164+
func migrateMovedFeaturesForChromiumHistograms(
166165
ctx context.Context,
167166
histogramsToEnumMap map[metricdatatypes.HistogramName]map[int64]*string,
168167
histogramsToAllFeatureKeySet map[metricdatatypes.HistogramName]map[string]metricdatatypes.HistogramEnumValue,
169168
movedFeatures map[string]web_platform_dx__web_features.FeatureMovedData,
170169
) error {
171170
for histogram, allFeaturesKeySet := range histogramsToAllFeatureKeySet {
172-
for featureKey, featureEnum := range allFeaturesKeySet {
173-
if movedFeatureData, found := movedFeatures[featureKey]; found {
174-
if _, exists := allFeaturesKeySet[movedFeatureData.RedirectTarget]; exists {
175-
// This new key already exists somewhere in the data.
176-
// That means upstream has done a partial migration to the new feature key.
177-
// Instead of assuming, we should error out and Chromium mojom file needs to be updated.
178-
slog.ErrorContext(ctx, "conflict migrating feature key. upstream currently using both keys",
179-
"histogram", histogram,
180-
"old_key", featureKey,
181-
"new_key", movedFeatureData.RedirectTarget,
182-
)
183-
184-
return ErrConflictMigratingFeatureKey
185-
}
186-
slog.WarnContext(ctx, "migrating feature key for histogram",
187-
"histogram", histogram,
188-
"old_key", featureKey,
189-
"new_key", movedFeatureData.RedirectTarget,
190-
)
191-
histogramsToEnumMap[histogram][featureEnum.Value] = &movedFeatureData.RedirectTarget
192-
}
171+
logger := slog.With("histogram", histogram)
172+
173+
err := NewMigrator(
174+
movedFeatures,
175+
allFeaturesKeySet,
176+
histogramsToEnumMap[histogram],
177+
WithLoggerForMigrator[metricdatatypes.HistogramEnumValue, map[int64]*string](logger),
178+
).Migrate(ctx, func(oldKey, newKey string, data map[int64]*string) {
179+
data[allFeaturesKeySet[oldKey].Value] = &newKey
180+
})
181+
if err != nil {
182+
return err
193183
}
194184
}
195185

‎lib/gcpspanner/spanneradapters/chromium_historgram_enum_consumer_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -290,7 +290,7 @@ func TestCreateEnumToFeatureKeyMap(t *testing.T) {
290290
}
291291
}
292292

293-
func TestMigrateMovedFeatures(t *testing.T) {
293+
func TestMigrateMovedFeaturesForChromiumHistograms(t *testing.T) {
294294
testCases := []struct {
295295
name string
296296
histogramsToEnumMap map[metricdatatypes.HistogramName]map[int64]*string
@@ -410,7 +410,7 @@ func TestMigrateMovedFeatures(t *testing.T) {
410410

411411
for _, tc := range testCases {
412412
t.Run(tc.name, func(t *testing.T) {
413-
err := migrateMovedFeatures(
413+
err := migrateMovedFeaturesForChromiumHistograms(
414414
context.Background(), tc.histogramsToEnumMap, tc.histogramsToAllFeatureKeySet, tc.movedFeatures)
415415

416416
if !errors.Is(err, tc.expectedErr) {

‎lib/gcpspanner/spanneradapters/developer_signals_consumer.go‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import (
1919

2020
"github.com/GoogleChrome/webstatus.dev/lib/developersignaltypes"
2121
"github.com/GoogleChrome/webstatus.dev/lib/gcpspanner"
22+
"github.com/GoogleChrome/webstatus.dev/lib/gen/jsonschema/web_platform_dx__web_features"
2223
)
2324

2425
// DeveloperSignalsConsumer handles the conversion of the developer signals between the downloaded
@@ -35,9 +36,36 @@ func NewDeveloperSignalsConsumer(client DeveloperSignalsClient) *DeveloperSignal
3536
// DeveloperSignalsClient expects a subset of the functionality from lib/gcpspanner that only apply to
3637
// Developer Signals.
3738
type DeveloperSignalsClient interface {
39+
GetAllMovedWebFeatures(ctx context.Context) ([]gcpspanner.MovedWebFeature, error)
3840
SyncLatestFeatureDeveloperSignals(ctx context.Context, data []gcpspanner.FeatureDeveloperSignal) error
3941
}
4042

43+
func (c *DeveloperSignalsConsumer) GetAllMovedWebFeatures(
44+
ctx context.Context) (map[string]web_platform_dx__web_features.FeatureMovedData, error) {
45+
movedFeatures, err := c.client.GetAllMovedWebFeatures(ctx)
46+
if err != nil {
47+
return nil, err
48+
}
49+
50+
return convertGCPSpannerMovedFeaturesToMap(movedFeatures), nil
51+
}
52+
53+
func migrateMovedFeaturesForDeveloperSignals(
54+
ctx context.Context,
55+
data *developersignaltypes.FeatureDeveloperSignals,
56+
movedFeatures map[string]web_platform_dx__web_features.FeatureMovedData) error {
57+
allFeaturesSet := make(map[string]struct{}, len(*data))
58+
for featureID := range *data {
59+
allFeaturesSet[featureID] = struct{}{}
60+
}
61+
62+
return NewMigrator(movedFeatures, allFeaturesSet, data).Migrate(ctx,
63+
func(oldKey, newKey string, data *developersignaltypes.FeatureDeveloperSignals) {
64+
(*data)[newKey] = (*data)[oldKey]
65+
delete(*data, oldKey)
66+
})
67+
}
68+
4169
// SyncLatestFeatureDeveloperSignals handles the conversion of developer signals between the workflow/API input
4270
// format and the format used by the GCP Spanner client.
4371
func (c *DeveloperSignalsConsumer) SyncLatestFeatureDeveloperSignals(
@@ -46,6 +74,20 @@ func (c *DeveloperSignalsConsumer) SyncLatestFeatureDeveloperSignals(
4674
return nil
4775
}
4876

77+
// Get all moved web features so we can migrate old data to the new feature ID.
78+
// We do this here because we want to avoid doing this for every metric.
79+
// We also want to avoid doing this in the spanner client because we want to
80+
// keep the spanner client as a generic library.
81+
movedFeatures, err := c.GetAllMovedWebFeatures(ctx)
82+
if err != nil {
83+
return err
84+
}
85+
86+
err = migrateMovedFeaturesForDeveloperSignals(ctx, data, movedFeatures)
87+
if err != nil {
88+
return err
89+
}
90+
4991
signals := make([]gcpspanner.FeatureDeveloperSignal, 0, len(*data))
5092
for featureID, signal := range *data {
5193
signals = append(signals, gcpspanner.FeatureDeveloperSignal{

‎lib/gcpspanner/spanneradapters/developer_signals_consumer_test.go‎

Lines changed: 119 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -25,96 +25,163 @@ import (
2525
"github.com/google/go-cmp/cmp"
2626
)
2727

28+
type SyncLatestFeatureDeveloperSignalsConfig struct {
29+
expectedData []gcpspanner.FeatureDeveloperSignal
30+
err error
31+
}
32+
2833
type MockDeveloperSignalsClient struct {
29-
callHistory []gcpspanner.FeatureDeveloperSignal
30-
shouldFail bool
31-
failWithError error
34+
// Config for GetAllMovedWebFeatures
35+
GetAllMovedWebFeaturesConfig *GetAllMovedWebFeaturesConfig
36+
37+
// Config for SyncLatestFeatureDeveloperSignals
38+
SyncLatestFeatureDeveloperSignalsConfig *SyncLatestFeatureDeveloperSignalsConfig
39+
t *testing.T
40+
}
41+
42+
func (m *MockDeveloperSignalsClient) GetAllMovedWebFeatures(_ context.Context) ([]gcpspanner.MovedWebFeature, error) {
43+
return m.GetAllMovedWebFeaturesConfig.output, m.GetAllMovedWebFeaturesConfig.err
3244
}
3345

3446
func (m *MockDeveloperSignalsClient) SyncLatestFeatureDeveloperSignals(
3547
_ context.Context,
3648
data []gcpspanner.FeatureDeveloperSignal,
3749
) error {
38-
m.callHistory = append(m.callHistory, data...)
39-
if m.shouldFail {
40-
return m.failWithError
50+
// Sort slices for deterministic comparison
51+
cmpFunc := func(i, j gcpspanner.FeatureDeveloperSignal) int {
52+
if i.WebFeatureKey < j.WebFeatureKey {
53+
return -1
54+
}
55+
if i.WebFeatureKey > j.WebFeatureKey {
56+
return 1
57+
}
58+
59+
return 0
60+
}
61+
slices.SortFunc(data, cmpFunc)
62+
slices.SortFunc(m.SyncLatestFeatureDeveloperSignalsConfig.expectedData, cmpFunc)
63+
if diff := cmp.Diff(m.SyncLatestFeatureDeveloperSignalsConfig.expectedData, data); diff != "" {
64+
m.t.Errorf("unexpected data (-want +got): %s", diff)
4165
}
4266

43-
return nil
67+
return m.SyncLatestFeatureDeveloperSignalsConfig.err
4468
}
4569

4670
func TestSyncLatestFeatureDeveloperSignals(t *testing.T) {
71+
var fakeGetAllMovedWebFeaturesError = errors.New("fake error")
72+
var fakeSyncError = errors.New("fake sync error")
73+
4774
testCases := []struct {
48-
name string
49-
input *developersignaltypes.FeatureDeveloperSignals
50-
mockClient *MockDeveloperSignalsClient
51-
expectedError error
52-
expectedCalls []gcpspanner.FeatureDeveloperSignal
75+
name string
76+
input *developersignaltypes.FeatureDeveloperSignals
77+
syncConfig *SyncLatestFeatureDeveloperSignalsConfig
78+
allMovedFeaturesConfig *GetAllMovedWebFeaturesConfig
79+
expectedError error
5380
}{
5481
{
5582
name: "Success",
5683
input: &developersignaltypes.FeatureDeveloperSignals{
5784
"feature1": {Upvotes: 100, Link: "link1"},
5885
"feature2": {Upvotes: 200, Link: "link2"},
5986
},
60-
mockClient: &MockDeveloperSignalsClient{callHistory: nil, shouldFail: false, failWithError: nil},
61-
expectedError: nil,
62-
expectedCalls: []gcpspanner.FeatureDeveloperSignal{
63-
{WebFeatureKey: "feature1", Upvotes: 100, Link: "link1"},
64-
{WebFeatureKey: "feature2", Upvotes: 200, Link: "link2"},
87+
syncConfig: &SyncLatestFeatureDeveloperSignalsConfig{
88+
expectedData: []gcpspanner.FeatureDeveloperSignal{
89+
{WebFeatureKey: "feature1", Upvotes: 100, Link: "link1"},
90+
{WebFeatureKey: "feature2", Upvotes: 200, Link: "link2"},
91+
},
92+
err: nil,
6593
},
94+
allMovedFeaturesConfig: &GetAllMovedWebFeaturesConfig{
95+
output: []gcpspanner.MovedWebFeature{},
96+
err: nil,
97+
},
98+
expectedError: nil,
6699
},
67100
{
68-
name: "Empty input",
69-
input: &developersignaltypes.FeatureDeveloperSignals{},
70-
mockClient: &MockDeveloperSignalsClient{callHistory: nil, shouldFail: false, failWithError: nil},
71-
expectedError: nil,
72-
expectedCalls: nil,
101+
name: "Empty input",
102+
input: &developersignaltypes.FeatureDeveloperSignals{},
103+
expectedError: nil,
104+
syncConfig: nil,
105+
allMovedFeaturesConfig: nil,
73106
},
74107
{
75-
name: "Spanner client error",
108+
name: "Spanner client error on sync",
76109
input: &developersignaltypes.FeatureDeveloperSignals{
77110
"feature1": {Upvotes: 100, Link: "link1"},
78111
},
79-
mockClient: &MockDeveloperSignalsClient{
80-
shouldFail: true,
81-
failWithError: errors.New("spanner error"),
82-
callHistory: nil,
112+
syncConfig: &SyncLatestFeatureDeveloperSignalsConfig{
113+
expectedData: []gcpspanner.FeatureDeveloperSignal{
114+
{WebFeatureKey: "feature1", Upvotes: 100, Link: "link1"},
115+
},
116+
err: fakeSyncError,
117+
},
118+
allMovedFeaturesConfig: &GetAllMovedWebFeaturesConfig{
119+
output: []gcpspanner.MovedWebFeature{},
120+
err: nil,
121+
},
122+
expectedError: fakeSyncError,
123+
},
124+
{
125+
name: "Error getting moved features",
126+
input: &developersignaltypes.FeatureDeveloperSignals{
127+
"feature1": {Upvotes: 100, Link: "link1"},
83128
},
84-
expectedError: errors.New("spanner error"),
85-
expectedCalls: []gcpspanner.FeatureDeveloperSignal{
86-
{WebFeatureKey: "feature1", Upvotes: 100, Link: "link1"},
129+
allMovedFeaturesConfig: &GetAllMovedWebFeaturesConfig{
130+
output: nil,
131+
err: fakeGetAllMovedWebFeaturesError,
87132
},
133+
syncConfig: nil,
134+
expectedError: fakeGetAllMovedWebFeaturesError,
135+
},
136+
{
137+
name: "Conflict with moved feature",
138+
input: &developersignaltypes.FeatureDeveloperSignals{
139+
"feature1": {Upvotes: 100, Link: "link1"}, // This one is moved
140+
"feature2": {Upvotes: 200, Link: "link2"}, // This is the new one
141+
},
142+
allMovedFeaturesConfig: &GetAllMovedWebFeaturesConfig{
143+
output: []gcpspanner.MovedWebFeature{
144+
{OriginalFeatureKey: "feature1", NewFeatureKey: "feature2"},
145+
},
146+
err: nil,
147+
},
148+
expectedError: ErrConflictMigratingFeatureKey,
149+
syncConfig: nil,
150+
},
151+
{
152+
name: "Success with moved feature",
153+
input: &developersignaltypes.FeatureDeveloperSignals{
154+
"feature1": {Upvotes: 100, Link: "link1"}, // This one is moved
155+
"feature3": {Upvotes: 300, Link: "link3"}, // This one is not moved
156+
},
157+
allMovedFeaturesConfig: &GetAllMovedWebFeaturesConfig{
158+
output: []gcpspanner.MovedWebFeature{
159+
{OriginalFeatureKey: "feature1", NewFeatureKey: "feature2"},
160+
},
161+
err: nil,
162+
},
163+
syncConfig: &SyncLatestFeatureDeveloperSignalsConfig{
164+
expectedData: []gcpspanner.FeatureDeveloperSignal{
165+
{WebFeatureKey: "feature2", Upvotes: 100, Link: "link1"},
166+
{WebFeatureKey: "feature3", Upvotes: 300, Link: "link3"},
167+
},
168+
err: nil,
169+
},
170+
expectedError: nil,
88171
},
89172
}
90173

91174
for _, tc := range testCases {
92175
t.Run(tc.name, func(t *testing.T) {
93-
consumer := NewDeveloperSignalsConsumer(tc.mockClient)
176+
consumer := NewDeveloperSignalsConsumer(&MockDeveloperSignalsClient{
177+
GetAllMovedWebFeaturesConfig: tc.allMovedFeaturesConfig,
178+
SyncLatestFeatureDeveloperSignalsConfig: tc.syncConfig,
179+
t: t,
180+
})
94181
err := consumer.SyncLatestFeatureDeveloperSignals(context.Background(), tc.input)
95182

96183
if !errors.Is(err, tc.expectedError) {
97-
if err.Error() != tc.expectedError.Error() {
98-
t.Errorf("unexpected error. got %v, want %v", err, tc.expectedError)
99-
}
100-
}
101-
102-
// Sort slices for deterministic comparison
103-
cmpFunc := func(i, j gcpspanner.FeatureDeveloperSignal) int {
104-
if i.WebFeatureKey < j.WebFeatureKey {
105-
return -1
106-
}
107-
if i.WebFeatureKey > j.WebFeatureKey {
108-
return 1
109-
}
110-
111-
return 0
112-
}
113-
slices.SortFunc(tc.mockClient.callHistory, cmpFunc)
114-
slices.SortFunc(tc.expectedCalls, cmpFunc)
115-
116-
if diff := cmp.Diff(tc.expectedCalls, tc.mockClient.callHistory); diff != "" {
117-
t.Errorf("unexpected call history (-want +got): %s", diff)
184+
t.Errorf("expected error %v, got %v", tc.expectedError, err)
118185
}
119186
})
120187
}

0 commit comments

Comments
 (0)