[v0/v1 migration] FilterStatVarsByEntity migration#1898
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements the FilterStatVarsByEntity method across the server architecture, including updates to the DataSource interface, dispatcher, and V2/V3 handlers. Specific implementations were added for Spanner, remote clients, and SQL data sources, supported by new unit tests and golden files. Review feedback suggests refining the response merging logic in DataSources to prevent potential nil pointer dereferences and optimizing the Spanner implementation with an early return when the input variable list is empty to avoid unnecessary database queries.
…cated merger function (consistent with other similar functions)
| resp := &pb.FilterStatVarsByEntityResponse{} | ||
| err := util.FetchRemote(rc.metadata, rc.httpClient, "/v2/variable/filter", req, resp) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
nit: consider logging an error here before returning
|
|
||
| rows, err := sds.client.CheckVariableExistence(ctx, ids, entities) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error checking variable existence: %w", err) |
There was a problem hiding this comment.
nit: either log an error here or ensure that that lower level method CheckVariableExistence has adequate logging.
Issue
b/513324218
Description
The
api/stats/stat-var-searchwas broken under the following circumstances:No results were returned in these circumstances (whereas results were successfully filtered by sv/entity observation existence and returned when using BT).
The reason for this was that
FilterStatVarsByEntityhad not been migrated and was relying on the existence of the BT cache.This PR creates a v3 (and diverted v2) implementation of the FilterStatVarsByEntity.
Testing
The problem is immediately visible on the dev scatter tool.
You can replicate it by:
Performing the same steps on production will give you a list of farm related stat vars that have been filtered to show only ones relevant to the place chosen.
Local Testing
To replicate the scenario we see in dev, you can start mixer with:
The key flag is
use_base_bigtable. By setting it to false, we disable BT, and in fact, we should be doing all our local testing this way if we want to truly test a Spanner only environment.With big table set to false, in master you will see the same error as in dev. In this branch, you will see filtered results that should be identical to those found in production.