Skip to content

Commit e0650a3

Browse files
authored
bugfix: Fix mapping of enums to feature IDs (#1538)
The instructions in the mojom file[1](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/mojom/use_counter/metrics/webdx_feature.mojom;l=35-47;drc=822a70f9ac61a75babe9d24ddfc32ab475acc7e1) were updated to reflect how feature developers should translate feature IDs found in the web features repo to enum labels in the mojom file by doing the equivalent of `id.title().replace("-", "")` However, our code takes the enum labels and tries to map convert labels to feature IDs. Unfortunately, there's too much data loss to reliably convert back to the feature ID. Examples: * **Case 1: `http3` (WebDX ID) -> `Http3` (Enum Label)** The original ID `http3` has **no hyphen** between the letter 'p' and the digit '3'. A reverse algorithm designed to insert hyphens (e.g., between letters and digits) would incorrectly produce `http-3`. * **Case 2: `canvas-2d-color-management` (WebDX ID) -> `Canvas2DColorManagement` (Enum Label)** Here, the original ID *does* have hyphens (e.g., `canvas-2d`). However, the enum label `Canvas2DColorManagement` provides no explicit signal for these specific hyphens. An algorithm that didn't insert hyphens (to avoid breaking `http3`) would then fail to restore them. As a result, we now get all the feature IDs, convert them in their respective labels and try to use them. There are some labels that have special cases. We handle those as well with a short special cases mapping.
1 parent 4950ffc commit e0650a3

6 files changed

Lines changed: 150 additions & 69 deletions

File tree

lib/gcpspanner/browser_feature_support_event.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,8 @@ func calculateBrowserSupportEvents(
101101
// PrecalculateBrowserFeatureSupportEvents populates the BrowserFeatureSupportEvents table with pre-calculated data.
102102
func (c *Client) PrecalculateBrowserFeatureSupportEvents(ctx context.Context, startAt, endAt time.Time) error {
103103
txn := c.ReadOnlyTransaction()
104+
defer txn.Close()
105+
104106
// 1. Fetch all BrowserFeatureAvailabilities
105107
availabilities, err := c.fetchAllBrowserAvailabilitiesWithTransaction(ctx, txn)
106108
if err != nil {

lib/gcpspanner/spanneradapters/chromium_historgram_enum_consumer.go

Lines changed: 46 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -19,10 +19,11 @@ import (
1919
"errors"
2020
"log/slog"
2121
"strings"
22-
"unicode"
2322

2423
"github.com/GoogleChrome/webstatus.dev/lib/gcpspanner"
2524
"github.com/GoogleChrome/webstatus.dev/lib/metricdatatypes"
25+
"golang.org/x/text/cases"
26+
"golang.org/x/text/language"
2627
)
2728

2829
// ChromiumHistogramEnumConsumer handles the conversion of histogram between the workflow/API input
@@ -43,10 +44,20 @@ type ChromiumHistogramEnumsClient interface {
4344
UpsertChromiumHistogramEnumValue(context.Context, gcpspanner.ChromiumHistogramEnumValue) (*string, error)
4445
UpsertWebFeatureChromiumHistogramEnumValue(context.Context, gcpspanner.WebFeatureChromiumHistogramEnumValue) error
4546
GetIDFromFeatureKey(context.Context, *gcpspanner.FeatureIDFilter) (*string, error)
47+
FetchAllFeatureKeys(context.Context) ([]string, error)
4648
}
4749

50+
// Used by GCP Log-based metrics to extract the data about mismatch mappings.
51+
const logMissingFeatureIDMetricMsg = "unable to find feature ID. skipping mapping"
52+
4853
func (c *ChromiumHistogramEnumConsumer) SaveHistogramEnums(
4954
ctx context.Context, data metricdatatypes.HistogramMapping) error {
55+
featureKeys, err := c.client.FetchAllFeatureKeys(ctx)
56+
if err != nil {
57+
return errors.Join(ErrFailedToGetFeatureKeys, err)
58+
}
59+
enumToFeatureKeyMap := createEnumToFeatureKeyMap(featureKeys)
60+
// Create mapping of anticipated enums to feature keys
5061
for histogram, enums := range data {
5162
enumID, err := c.client.UpsertChromiumHistogramEnum(ctx, gcpspanner.ChromiumHistogramEnum{
5263
HistogramName: string(histogram),
@@ -63,12 +74,21 @@ func (c *ChromiumHistogramEnumConsumer) SaveHistogramEnums(
6374
if err != nil {
6475
return errors.Join(ErrFailedToStoreEnumValue, err)
6576
}
66-
featureKey := enumLabelToFeatureKey(enum.Label)
77+
78+
featureKey, found := enumToFeatureKeyMap[enum.Label]
79+
if !found {
80+
slog.WarnContext(ctx,
81+
logMissingFeatureIDMetricMsg,
82+
"label", enum.Label)
83+
84+
continue
85+
}
86+
6787
featureID, err := c.client.GetIDFromFeatureKey(
6888
ctx, gcpspanner.NewFeatureKeyFilter(featureKey))
6989
if err != nil {
7090
slog.WarnContext(ctx,
71-
"unable to find feature ID. skipping mapping",
91+
logMissingFeatureIDMetricMsg,
7292
"error", err,
7393
"featureKey", featureKey,
7494
"label", enum.Label)
@@ -97,29 +117,35 @@ var (
97117
// the mapping between enum value and web feature.
98118
ErrFailedToStoreEnumValueWebFeatureMapping = errors.New(
99119
"failed to store web feature to chromium enum value mapping")
120+
// ErrFailedToGetFeatureKeys indicates an internal error when trying to get all the feature keys.
121+
ErrFailedToGetFeatureKeys = errors.New("failed to get feature keys")
100122
)
101123

102-
func enumLabelToFeatureKey(label string) string {
103-
b := strings.Builder{}
104-
for idx, c := range label {
105-
// First character just return the lower case version of it.
106-
if idx == 0 {
107-
b.WriteRune(unicode.ToLower(c))
124+
// nolint:lll // WONTFIX: useful comment message
125+
// createEnumToFeatureKeyMap uses the list of WebDX feature keys to
126+
// generate a map from the enum label (e.g., "ViewTransitions")
127+
// back to its original WebDX feature key (e.g., "view-transitions").
128+
// It uses the same transformation logic described in the Chromium mojom file.
129+
// https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/mojom/use_counter/metrics/webdx_feature.mojom;l=35-47;drc=822a70f9ac61a75babe9d24ddfc32ab475acc7e1
130+
func createEnumToFeatureKeyMap(featureKeys []string) map[string]string {
131+
titleCaser := cases.Title(language.English)
132+
m := make(map[string]string, len(featureKeys))
133+
specialCases := map[string]string{
134+
"float16array": "Float16Array",
135+
"uint8array-base64-hex": "Uint8ArrayBase64Hex",
136+
}
137+
for _, featureKey := range featureKeys {
138+
if specialCaseLabel, found := specialCases[featureKey]; found {
139+
m[specialCaseLabel] = featureKey
108140

109141
continue
110142
}
111-
if unicode.IsUpper(c) {
112-
b.WriteRune('-')
113-
b.WriteRune(unicode.ToLower(c))
114143

115-
continue
116-
}
117-
// Add hyphen if previous character is a letter and current character is a digit
118-
if idx > 0 && unicode.IsLetter(rune(label[idx-1])) && unicode.IsDigit(c) {
119-
b.WriteRune('-')
120-
}
121-
b.WriteRune(c)
144+
enumLabel := titleCaser.String(featureKey)
145+
enumLabel = strings.ReplaceAll(enumLabel, "-", "")
146+
// Before storing it, check if it exists
147+
m[enumLabel] = featureKey
122148
}
123149

124-
return b.String()
150+
return m
125151
}

lib/gcpspanner/spanneradapters/chromium_historgram_enum_consumer_test.go

Lines changed: 68 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ package spanneradapters
1717
import (
1818
"context"
1919
"errors"
20+
"reflect"
2021
"testing"
2122

2223
"github.com/GoogleChrome/webstatus.dev/lib/gcpspanner"
@@ -31,6 +32,7 @@ type mockChromiumHistogramEnumsClient struct {
3132
upsertWebFeatureChromiumHistogramEnumValue func(context.Context,
3233
gcpspanner.WebFeatureChromiumHistogramEnumValue) error
3334
getIDFromFeatureKey func(context.Context, *gcpspanner.FeatureIDFilter) (*string, error)
35+
fetchAllFeatureKeys func(context.Context) ([]string, error)
3436
}
3537

3638
func (m *mockChromiumHistogramEnumsClient) UpsertChromiumHistogramEnum(ctx context.Context,
@@ -53,6 +55,11 @@ func (m *mockChromiumHistogramEnumsClient) GetIDFromFeatureKey(ctx context.Conte
5355
return m.getIDFromFeatureKey(ctx, in)
5456
}
5557

58+
func (m *mockChromiumHistogramEnumsClient) FetchAllFeatureKeys(
59+
ctx context.Context) ([]string, error) {
60+
return m.fetchAllFeatureKeys(ctx)
61+
}
62+
5663
func TestChromiumHistogramEnumConsumer_SaveHistogramEnums(t *testing.T) {
5764
tests := []struct {
5865
name string
@@ -63,6 +70,9 @@ func TestChromiumHistogramEnumConsumer_SaveHistogramEnums(t *testing.T) {
6370
{
6471
name: "Success",
6572
client: &mockChromiumHistogramEnumsClient{
73+
fetchAllFeatureKeys: func(_ context.Context) ([]string, error) {
74+
return []string{"enum-label"}, nil
75+
},
6676
upsertChromiumHistogramEnum: func(_ context.Context,
6777
_ gcpspanner.ChromiumHistogramEnum) (*string, error) {
6878
return valuePtr("enumID"), nil
@@ -87,9 +97,30 @@ func TestChromiumHistogramEnumConsumer_SaveHistogramEnums(t *testing.T) {
8797
},
8898
expectedErr: nil,
8999
},
100+
{
101+
name: "FetchAllFeatureKeys returns error",
102+
client: &mockChromiumHistogramEnumsClient{
103+
fetchAllFeatureKeys: func(_ context.Context) ([]string, error) {
104+
return nil, errors.New("test error")
105+
},
106+
upsertChromiumHistogramEnum: nil,
107+
upsertChromiumHistogramEnumValue: nil,
108+
upsertWebFeatureChromiumHistogramEnumValue: nil,
109+
getIDFromFeatureKey: nil,
110+
},
111+
data: metricdatatypes.HistogramMapping{
112+
metricdatatypes.WebDXFeatureEnum: []metricdatatypes.HistogramEnumValue{
113+
{Value: 1, Label: "EnumLabel"},
114+
},
115+
},
116+
expectedErr: ErrFailedToGetFeatureKeys,
117+
},
90118
{
91119
name: "UpsertChromiumHistogramEnum returns error",
92120
client: &mockChromiumHistogramEnumsClient{
121+
fetchAllFeatureKeys: func(_ context.Context) ([]string, error) {
122+
return []string{"enum-label"}, nil
123+
},
93124
upsertChromiumHistogramEnum: func(_ context.Context,
94125
_ gcpspanner.ChromiumHistogramEnum) (*string, error) {
95126
return nil, errors.New("test error")
@@ -108,6 +139,9 @@ func TestChromiumHistogramEnumConsumer_SaveHistogramEnums(t *testing.T) {
108139
{
109140
name: "UpsertChromiumHistogramEnumValue returns error",
110141
client: &mockChromiumHistogramEnumsClient{
142+
fetchAllFeatureKeys: func(_ context.Context) ([]string, error) {
143+
return []string{"enum-label"}, nil
144+
},
111145
upsertChromiumHistogramEnum: func(_ context.Context,
112146
_ gcpspanner.ChromiumHistogramEnum) (*string, error) {
113147
return valuePtr("enumID"), nil
@@ -129,6 +163,9 @@ func TestChromiumHistogramEnumConsumer_SaveHistogramEnums(t *testing.T) {
129163
{
130164
name: "GetIDFromFeatureKey returns error",
131165
client: &mockChromiumHistogramEnumsClient{
166+
fetchAllFeatureKeys: func(_ context.Context) ([]string, error) {
167+
return []string{"enum-label"}, nil
168+
},
132169
upsertChromiumHistogramEnum: func(_ context.Context,
133170
_ gcpspanner.ChromiumHistogramEnum) (*string, error) {
134171
return valuePtr("enumID"), nil
@@ -153,6 +190,9 @@ func TestChromiumHistogramEnumConsumer_SaveHistogramEnums(t *testing.T) {
153190
{
154191
name: "UpsertWebFeatureChromiumHistogramEnumValue returns error",
155192
client: &mockChromiumHistogramEnumsClient{
193+
fetchAllFeatureKeys: func(_ context.Context) ([]string, error) {
194+
return []string{"enum-label"}, nil
195+
},
156196
upsertChromiumHistogramEnum: func(_ context.Context,
157197
_ gcpspanner.ChromiumHistogramEnum) (*string, error) {
158198
return valuePtr("enumID"), nil
@@ -195,48 +235,34 @@ func TestChromiumHistogramEnumConsumer_SaveHistogramEnums(t *testing.T) {
195235
}
196236
}
197237

198-
func TestEnumLabelToFeatureKey(t *testing.T) {
199-
tests := []struct {
200-
name string
201-
label string
202-
want string
203-
}{
204-
{
205-
name: "Simple lowercase",
206-
label: "simple",
207-
want: "simple",
208-
},
209-
{
210-
name: "typical",
211-
label: "TypicalCase",
212-
want: "typical-case",
213-
},
214-
{
215-
name: "With numbers in the middle",
216-
label: "With123Numbers",
217-
want: "with-123-numbers",
218-
},
219-
{
220-
name: "Starting with number",
221-
label: "123Abc",
222-
want: "123-abc",
223-
},
224-
{
225-
name: "Consecutive uppercase letters",
226-
label: "ABCTest",
227-
want: "a-b-c-test",
228-
},
229-
{
230-
name: "Mixed case with numbers and consecutive uppercase",
231-
label: "ABC123defGHI456Jkl",
232-
want: "a-b-c-123def-g-h-i-456-jkl",
233-
},
238+
func TestCreateEnumToFeatureKeyMap(t *testing.T) {
239+
featureKeys := []string{
240+
"canvas-2d-color-management",
241+
"http3",
242+
"intersection-observer-v2",
243+
"view-transitions",
244+
// Special cases
245+
"float16array",
246+
"uint8array-base64-hex",
234247
}
235-
for _, tc := range tests {
236-
t.Run(tc.name, func(t *testing.T) {
237-
if got := enumLabelToFeatureKey(tc.label); got != tc.want {
238-
t.Errorf("enumLabelToFeatureKey() = %v, want %v", got, tc.want)
239-
}
240-
})
248+
// nolint: lll // WONTFIX: useful comment with SHA
249+
want := map[string]string{
250+
"Canvas2DColorManagement": "canvas-2d-color-management",
251+
"Http3": "http3",
252+
"IntersectionObserverV2": "intersection-observer-v2",
253+
"ViewTransitions": "view-transitions",
254+
/*
255+
Special cases
256+
*/
257+
// https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/mojom/use_counter/metrics/webdx_feature.mojom;l=360;drc=822a70f9ac61a75babe9d24ddfc32ab475acc7e1
258+
// https://github.com/web-platform-dx/web-features/blob/main/features/float16array.yml
259+
"Float16Array": "float16array",
260+
// https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/mojom/use_counter/metrics/webdx_feature.mojom;l=396;drc=822a70f9ac61a75babe9d24ddfc32ab475acc7e1
261+
// https://github.com/web-platform-dx/web-features/blob/main/features/uint8array-base64-hex.yml
262+
"Uint8ArrayBase64Hex": "uint8array-base64-hex",
263+
}
264+
got := createEnumToFeatureKeyMap(featureKeys)
265+
if !reflect.DeepEqual(got, want) {
266+
t.Errorf("createEnumToFeatureKeyMap()\ngot: (%+v)\nwant: (%+v)\n", got, want)
241267
}
242268
}

lib/gcpspanner/web_features.go

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -102,21 +102,33 @@ func (c *Client) GetIDFromFeatureKey(ctx context.Context, filter *FeatureIDFilte
102102

103103
func (c *Client) fetchAllWebFeatureIDsWithTransaction(
104104
ctx context.Context, txn *spanner.ReadOnlyTransaction) ([]string, error) {
105-
var ids []string
106-
iter := txn.Read(ctx, webFeaturesTable, spanner.AllKeys(), []string{"ID"})
105+
return fetchColumnValuesWithTransaction[string](ctx, txn, webFeaturesTable, "ID")
106+
}
107+
108+
func (c *Client) FetchAllFeatureKeys(ctx context.Context) ([]string, error) {
109+
txn := c.ReadOnlyTransaction()
110+
defer txn.Close()
111+
112+
return fetchColumnValuesWithTransaction[string](ctx, txn, webFeaturesTable, "FeatureKey")
113+
}
114+
115+
func fetchColumnValuesWithTransaction[T any](
116+
ctx context.Context, txn *spanner.ReadOnlyTransaction, table string, columnName string) ([]T, error) {
117+
var values []T
118+
iter := txn.Read(ctx, table, spanner.AllKeys(), []string{columnName})
107119
defer iter.Stop()
108120
err := iter.Do(func(row *spanner.Row) error {
109-
var id string
110-
if err := row.Column(0, &id); err != nil {
121+
var value T
122+
if err := row.Column(0, &value); err != nil {
111123
return err
112124
}
113-
ids = append(ids, id)
125+
values = append(values, value)
114126

115127
return nil
116128
})
117129
if err != nil {
118130
return nil, err
119131
}
120132

121-
return ids, nil
133+
return values, nil
122134
}

lib/gcpspanner/web_features_test.go

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,4 +125,19 @@ func TestUpsertWebFeature(t *testing.T) {
125125
if !slices.Equal[[]WebFeature](expectedPageAfterUpdate, features) {
126126
t.Errorf("unequal features after update. expected %+v actual %+v", sampleFeatures, features)
127127
}
128+
129+
expectedKeys := []string{
130+
"feature1",
131+
"feature2",
132+
"feature3",
133+
"feature4",
134+
}
135+
keys, err := spannerClient.FetchAllFeatureKeys(ctx)
136+
if err != nil {
137+
t.Errorf("unexpected error fetching all keys")
138+
}
139+
slices.Sort(keys)
140+
if !slices.Equal(keys, expectedKeys) {
141+
t.Errorf("unequal keys. expected %+v actual %+v", expectedKeys, keys)
142+
}
128143
}

lib/go.mod

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,7 @@ require (
159159
golang.org/x/oauth2 v0.29.0
160160
golang.org/x/sync v0.13.0 // indirect
161161
golang.org/x/sys v0.32.0 // indirect
162-
golang.org/x/text v0.24.0 // indirect
162+
golang.org/x/text v0.24.0
163163
golang.org/x/time v0.11.0 // indirect
164164
google.golang.org/genproto v0.0.0-20250428153025-10db94c68c34 // indirect
165165
google.golang.org/genproto/googleapis/api v0.0.0-20250428153025-10db94c68c34 // indirect

0 commit comments

Comments
 (0)