Combine desktop and mobile for "Missing one" chart #1461

DanielRyanSmith · 2025-05-05T03:02:12Z

This change updates the "missing one" API endpoints to accept a new query parameter which will allow desktop and mobile versions of the same browser to be treated as one entity.

This should have no change on the app's current behavior, as the request for the front-end has not yet been changed.

jcscottiii

I know this is in draft. But I wanted to give some early feedback while I had a chance.

backend/pkg/httpserver/list_missing_one_implementation_counts.go

backend/pkg/httpserver/list_missing_one_implementation_counts_test.go

backend/pkg/httpserver/list_missing_one_implementation_counts.go

DanielRyanSmith · 2025-05-06T20:10:37Z

lib/gcpspanner/missing_one_implementation.go

+	var targetBrowserFilter string
+	if len(targetBrowsers) > 1 {
+		targetBrowserFilter = fmt.Sprintf("(bfse.TargetBrowserName = '%s' OR bfse.TargetBrowserName = '%s')",
+			targetBrowsers[0], targetBrowsers[1])
+	} else {
+		targetBrowserFilter = fmt.Sprintf("bfse.TargetBrowserName = '%s'", targetBrowsers[0])


I don't know if this is necessarily the best way to generate these filters, but the inputs are sanitized, so this shouldn't cause any problem. 😅

We should continue to treat each input as user input and add them as spanner parameters to the params map like before.

conditions := make([]string, len(targetBrowsers)) for i, browserName := range targetBrowsers { paramName := fmt.Sprintf("targetBrowserParam%d", i) // Create a unique param name, e.g., targetBrowserParam0 conditions[i] = fmt.Sprintf("bfse.TargetBrowserName = @%s", paramName) params[paramName] = browserName // Add the actual browser name to the params map } targetBrowserFilter = fmt.Sprintf("(%s)", strings.Join(conditions, " OR "))

Also, I thought we wanted the set of features that exist in both the desktop and mobile (vs either the desktop or mobile version). Would this current query do that?

If you want to revisit handling N cases, you can file an issue and do that later. But we should use the param map now just to be safe.

jcscottiii · 2025-05-06T20:23:17Z

lib/gcpspanner/missing_one_implementation.go

+	var targetBrowserFilter string
+	if len(targetBrowsers) > 1 {
+		targetBrowserFilter = fmt.Sprintf("(bfse.TargetBrowserName = '%s' OR bfse.TargetBrowserName = '%s')",
+			targetBrowsers[0], targetBrowsers[1])
+	} else {
+		targetBrowserFilter = fmt.Sprintf("bfse.TargetBrowserName = '%s'", targetBrowsers[0])


We should continue to treat each input as user input and add them as spanner parameters to the params map like before.

conditions := make([]string, len(targetBrowsers)) for i, browserName := range targetBrowsers { paramName := fmt.Sprintf("targetBrowserParam%d", i) // Create a unique param name, e.g., targetBrowserParam0 conditions[i] = fmt.Sprintf("bfse.TargetBrowserName = @%s", paramName) params[paramName] = browserName // Add the actual browser name to the params map } targetBrowserFilter = fmt.Sprintf("(%s)", strings.Join(conditions, " OR "))

jcscottiii · 2025-05-06T20:24:54Z

lib/gcpspanner/missing_one_implementation.go

+	var targetBrowserFilter string
+	if len(targetBrowsers) > 1 {
+		targetBrowserFilter = fmt.Sprintf("(bfse.TargetBrowserName = '%s' OR bfse.TargetBrowserName = '%s')",
+			targetBrowsers[0], targetBrowsers[1])
+	} else {
+		targetBrowserFilter = fmt.Sprintf("bfse.TargetBrowserName = '%s'", targetBrowsers[0])


Also, I thought we wanted the set of features that exist in both the desktop and mobile (vs either the desktop or mobile version). Would this current query do that?

jcscottiii · 2025-05-06T20:27:58Z

lib/gcpspanner/missing_one_implementation_feature_list.go

-	params["targetBrowserParam"] = targetBrowser
-	otherBrowsersParamNames := make([]string, 0, len(otherBrowsers))
+	var targetBrowserFilter string
+	if len(targetBrowsers) > 1 {


Refer to the earlier comment about using the param map.

jcscottiii · 2025-05-06T20:28:03Z

lib/gcpspanner/missing_one_implementation_feature_list.go

-		paramName := fmt.Sprintf("otherBrowser%d", i)
-		params[paramName] = otherBrowsers[i]
-		otherBrowsersParamNames = append(otherBrowsersParamNames, paramName)
+		if len(otherBrowsers[i]) > 1 {


Refer to the earlier comment about using the param map.

jcscottiii · 2025-05-06T20:30:41Z

lib/gcpspanner/missing_one_implementation.go

+	var targetBrowserFilter string
+	if len(targetBrowsers) > 1 {
+		targetBrowserFilter = fmt.Sprintf("(bfse.TargetBrowserName = '%s' OR bfse.TargetBrowserName = '%s')",
+			targetBrowsers[0], targetBrowsers[1])
+	} else {
+		targetBrowserFilter = fmt.Sprintf("bfse.TargetBrowserName = '%s'", targetBrowsers[0])


If you want to revisit handling N cases, you can file an issue and do that later. But we should use the param map now just to be safe.

jcscottiii · 2025-05-06T20:34:35Z

lib/gcpspanner/spanneradapters/backend_test.go

+type mockDesktopMobileBrowserPair struct {
+	Desktop string
+	Mobile  string
+}
+


Suggested change

type mockDesktopMobileBrowserPair struct {

Desktop string

Mobile string

}

I think you meant to remove this.

jcscottiii · 2025-05-07T20:41:30Z

backend/pkg/httpserver/list_missing_one_implementation_counts_test.go

+						`"endAt":"2000-01-10","include_baseline_mobile_browsers":true,` +
+						`"browser":["chrome","firefox","safari"]}}`,
+					Value: nil,
+					Err:   ErrNoMatchingMobileBrowser,


This is the error returned from the cache layer.

Suggested change

Err: ErrNoMatchingMobileBrowser,

Err: cachetypes.ErrCachedDataNotFound,

jcscottiii · 2025-05-07T20:42:58Z

backend/pkg/httpserver/list_missing_one_implementation_counts_test.go

+			mockConfig: &MockListMissingOneImplCountsConfig{
+				expectedTargetBrowsers: []string{"chrome"},
+				expectedOtherBrowsers: [][]string{
+					{"edge"},
+					{"firefox"},
+					{"safari"},
+				},
+				expectedStartAt:   time.Date(2000, time.January, 1, 0, 0, 0, 0, time.UTC),
+				expectedEndAt:     time.Date(2000, time.January, 10, 0, 0, 0, 0, time.UTC),
+				expectedPageSize:  100,
+				expectedPageToken: badPageToken,
+				page:              nil,
+				pageToken:         nil,
+				err:               backendtypes.ErrInvalidPageToken,
+			},


The validation logic should stop before the call to ListMissingOneImplCounts

Suggested change

mockConfig: &MockListMissingOneImplCountsConfig{

expectedTargetBrowsers: []string{"chrome"},

expectedOtherBrowsers: [][]string{

{"edge"},

{"firefox"},

{"safari"},

},

expectedStartAt: time.Date(2000, time.January, 1, 0, 0, 0, 0, time.UTC),

expectedEndAt: time.Date(2000, time.January, 10, 0, 0, 0, 0, time.UTC),

expectedPageSize: 100,

expectedPageToken: badPageToken,

page: nil,

pageToken: nil,

err: backendtypes.ErrInvalidPageToken,

},

mockConfig: nil,

jcscottiii · 2025-05-07T20:43:56Z

lib/gcpspanner/missing_one_implementation.go

+
+	targetBrowserConditions := make([]string, 0, len(targetBrowsers))
+	for i, browserName := range targetBrowsers {
+		paramName := fmt.Sprintf("targetBrowserParam%d", i) // Create a unique param name, e.g., targetBrowserParam0


Suggested change

paramName := fmt.Sprintf("targetBrowserParam%d", i) // Create a unique param name, e.g., targetBrowserParam0

paramName := fmt.Sprintf("targetBrowserParam%d", i)

jcscottiii · 2025-05-07T20:49:40Z

lib/gcpspanner/missing_one_implementation.go

@@ -103,9 +104,9 @@ SELECT releases.EventReleaseDate,
 			AND obsf.EventReleaseDate = tbuf.EventReleaseDate
 		WHERE
 			tbuf.EventReleaseDate = releases.EventReleaseDate
-			{{ range $browserParamName := .OtherBrowsersParamNames }}
+			{{ range $releaseDateFilter := .ReleaseDateFilters }}


Could you talk a little bit about the need for the releaseDateFilter? I'm trying to understand why it is needed now.

After reviewing the query again, I do not think this is needed, and I had mistakenly thought the browsers needed to be queried in their respective pairs here. I'm updating the code to reflect this change.

jcscottiii reviewed May 5, 2025

View reviewed changes

DanielRyanSmith marked this pull request as ready for review May 6, 2025 19:54

DanielRyanSmith commented May 6, 2025

View reviewed changes

jcscottiii reviewed May 6, 2025

View reviewed changes

DanielRyanSmith force-pushed the combine-missing-one branch from 14dcab6 to a6427d6 Compare May 7, 2025 17:28

jcscottiii reviewed May 7, 2025

View reviewed changes

DanielRyanSmith added 10 commits May 19, 2025 16:32

combine desktop and mobile for missing-one chart

446a65a

move logic to http layer

f43e136

move logic to http layer (WIP)

6fa8e3a

handle combined desktop and mobile in queries

38e434f

revert pw changes

82acc6a

lint

a83d1a9

lint

c9b9ec4

changes suggested by @jcscottiii

18ce89a

adjust array creation

13fa192

Changes suggested by @jcscottiii

8d703b8

DanielRyanSmith force-pushed the combine-missing-one branch from 1cb07ff to 8d703b8 Compare May 19, 2025 16:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Combine desktop and mobile for "Missing one" chart #1461

Combine desktop and mobile for "Missing one" chart #1461

DanielRyanSmith commented May 5, 2025 •

edited

Loading

jcscottiii left a comment

DanielRyanSmith May 6, 2025

jcscottiii May 6, 2025

jcscottiii May 6, 2025

jcscottiii May 6, 2025

jcscottiii May 6, 2025

jcscottiii May 6, 2025

jcscottiii May 6, 2025

jcscottiii May 6, 2025

jcscottiii May 6, 2025

jcscottiii May 6, 2025

jcscottiii May 6, 2025

jcscottiii May 7, 2025

jcscottiii May 7, 2025

jcscottiii May 7, 2025

jcscottiii May 7, 2025

DanielRyanSmith May 7, 2025

	Err: ErrNoMatchingMobileBrowser,
	Err: cachetypes.ErrCachedDataNotFound,

	paramName := fmt.Sprintf("targetBrowserParam%d", i) // Create a unique param name, e.g., targetBrowserParam0
	paramName := fmt.Sprintf("targetBrowserParam%d", i)

Combine desktop and mobile for "Missing one" chart #1461

Are you sure you want to change the base?

Combine desktop and mobile for "Missing one" chart #1461

Conversation

DanielRyanSmith commented May 5, 2025 • edited Loading

jcscottiii 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielRyanSmith commented May 5, 2025 •

edited

Loading