feat: Add flag for specifying the the VID model line

[kungfucraig]

Support VID Model Line rollout using Reporting Server flags. Two flags are added:

–default-vid-model-line=
Specifies the VID model line value to be used for the “model_line” field in MeasurementSpec This in turn determines what VID model line that EDPs will use to fulfill Requisitions. If empty EDPs must agree offline what the default is. Once enabled we encourage that this case become an error.

–measurement-consumer-vid-model-line=Map<String, String>
A map of MC names to model lines. When a Metric is created, if the Metric is for an MC present in this map, then the model line provided as the value will be used for the “model_line” field in MeasurementSpec.

[wfa-reviewable]

This change is 

[SanjayVas]

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @kungfucraig)

---

src/main/kotlin/org/wfanet/measurement/reporting/deploy/v2/common/ReportingApiServerFlags.kt line 58 at r1 (raw file):

  @CommandLine.Option(
    names = ["--measurement-consumer-model-lines"],

Singular flag name

Suggestion:

--measurement-consumer-model-line

---

src/main/kotlin/org/wfanet/measurement/reporting/deploy/v2/common/ReportingApiServerFlags.kt line 61 at r1 (raw file):

    description =
      [
        "A map of measurement consumer names to model lines. This map indicates which model line " +

Document that this is a key-value pair and that it can be specified multiple times. See --event-group-spec in SyntheticGeneratorEdpSimulatorRunner.kt as an example.

The flag usage is that you'd specify the flags multiple times, once for each key-value pair.

---

src/main/kotlin/org/wfanet/measurement/reporting/deploy/v2/common/ReportingApiServerFlags.kt line 61 at r1 (raw file):

    description =
      [
        "A map of measurement consumer names to model lines. This map indicates which model line " +

nit: be more explicit that this is a resource name

Suggestion:

resource name

---

src/test/kotlin/org/wfanet/measurement/reporting/service/api/v2alpha/MetricsServiceTest.kt line 493 at r1 (raw file):

  )

private val DEFAULT_VID_MODEL_LINE = "the-model-line"

const

---

src/test/kotlin/org/wfanet/measurement/reporting/service/api/v2alpha/MetricsServiceTest.kt line 495 at r1 (raw file):

private val DEFAULT_VID_MODEL_LINE = "the-model-line"
private val MEASUREMENT_CONSUMER_MODEL_LINES =
  mapOf<String, String>(MEASUREMENT_CONSUMERS.values.first().name to "mc-model-line")

nit: prefer specifying the property type and letting it use type inference for the builder types

Suggestion:

private val MEASUREMENT_CONSUMER_MODEL_LINES: Map<String, String> =
  mapOf(MEASUREMENT_CONSUMERS.values.first().name to "mc-model-line")

---

src/test/kotlin/org/wfanet/measurement/reporting/service/api/v2alpha/MetricsServiceTest.kt line 2461 at r1 (raw file):

      )
  }

Ideally you want a test that creates a Metric using a different MC so that you can verify that DEFAULT_VID_MODEL_LINE is used

---

src/test/kotlin/org/wfanet/measurement/reporting/service/api/v2alpha/MetricsServiceTest.kt line 2527 at r1 (raw file):

        createMeasurementRequest.measurement.measurementSpec.unpack()
      assertThat(measurementSpec)
        .isEqualTo(

Shouldn't this be failing since it doesn't check that the model line is set?

[kungfucraig]

Reviewable status: 2 of 4 files reviewed, 5 unresolved discussions (waiting on @SanjayVas)

---

src/main/kotlin/org/wfanet/measurement/reporting/deploy/v2/common/ReportingApiServerFlags.kt line 58 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Singular flag name

Done.

---

src/main/kotlin/org/wfanet/measurement/reporting/deploy/v2/common/ReportingApiServerFlags.kt line 61 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Document that this is a key-value pair and that it can be specified multiple times. See --event-group-spec in SyntheticGeneratorEdpSimulatorRunner.kt as an example.

The flag usage is that you'd specify the flags multiple times, once for each key-value pair.

Done.

---

src/test/kotlin/org/wfanet/measurement/reporting/service/api/v2alpha/MetricsServiceTest.kt line 493 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

const

Done.

---

src/test/kotlin/org/wfanet/measurement/reporting/service/api/v2alpha/MetricsServiceTest.kt line 2461 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Ideally you want a test that creates a Metric using a different MC so that you can verify that DEFAULT_VID_MODEL_LINE is used

That what I mentioned on chat. The setup for an MC is onerous. On a side, I expect we'd see this MC setup code copied and pasted in a lot of places by now and having some test utilities could be a nice thing. In our spare time I suppose...

Anyway, I built a second service that doesn't have the MC map to test the default model line behavior. That seems to work.

---

src/test/kotlin/org/wfanet/measurement/reporting/service/api/v2alpha/MetricsServiceTest.kt line 2527 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Shouldn't this be failing since it doesn't check that the model line is set?

No, it's comparing to modified copy of UNION_ALL_BUT_LAST SPEC, which is a copy of the BASE SPEC, which sets the model line.

[SanjayVas]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kungfucraig)

---

src/test/kotlin/org/wfanet/measurement/reporting/service/api/v2alpha/MetricsServiceTest.kt line 2463 at r2 (raw file):

    // create a second service with an empty measurement consumer model line map
    service2 =

This will create the second service for every test method. You want to instead create this service as a local variable within the test method that uses it.

[kungfucraig]

Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @SanjayVas)

---

src/test/kotlin/org/wfanet/measurement/reporting/service/api/v2alpha/MetricsServiceTest.kt line 2463 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

This will create the second service for every test method. You want to instead create this service as a local variable within the test method that uses it.

Done.

[SanjayVas]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @kungfucraig)

[stevenwarejones]

Reviewed all commit messages.
Reviewable status: 4 of 6 files reviewed, 1 unresolved discussion (waiting on @kungfucraig and @SanjayVas)

---

src/main/kotlin/org/wfanet/measurement/reporting/deploy/v2/common/ReportingApiServerFlags.kt line 58 at r4 (raw file):

  @CommandLine.Option(
    names = ["--measurement-consumer-model-line"],

this feels pretty arduous to maintain.

A different design would automatically set the default for a MC the first time they run a measurement and store that value to continue using until updated at some point in the future.

[kungfucraig]

Reviewable status: 4 of 6 files reviewed, 1 unresolved discussion (waiting on @SanjayVas and @stevenwarejones)

---

src/main/kotlin/org/wfanet/measurement/reporting/deploy/v2/common/ReportingApiServerFlags.kt line 58 at r4 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

this feels pretty arduous to maintain.

A different design would automatically set the default for a MC the first time they run a measurement and store that value to continue using until updated at some point in the future.

This flag is only meant to enable safe model rollouts. Specifically it is to allow a specific test MC to have a new model enabled before the model is enabled as the default for all MCs. HMSS has an almost identical configuration, and I anticipate we'll use this pattern a lot in the future (i.e. default config with per MC overrides to test rollouts).

Also note that, the use of the VID Model Repo will replace this entire PR.

[stevenwarejones]

Reviewable status: 4 of 6 files reviewed, 1 unresolved discussion (waiting on @kungfucraig and @SanjayVas)

---

src/main/kotlin/org/wfanet/measurement/reporting/deploy/v2/common/ReportingApiServerFlags.kt line 58 at r4 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

This flag is only meant to enable safe model rollouts. Specifically it is to allow a specific test MC to have a new model enabled before the model is enabled as the default for all MCs. HMSS has an almost identical configuration, and I anticipate we'll use this pattern a lot in the future (i.e. default config with per MC overrides to test rollouts).

Also note that, the use of the VID Model Repo will replace this entire PR.

this requires restarting the reporting server each time then, right? That also seems kind of messy here. Shouldn't this be set when the individual report is requested then?

[kungfucraig]

Reviewable status: 4 of 6 files reviewed, 1 unresolved discussion (waiting on @SanjayVas and @stevenwarejones)

---

src/main/kotlin/org/wfanet/measurement/reporting/deploy/v2/common/ReportingApiServerFlags.kt line 58 at r4 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

this requires restarting the reporting server each time then, right? That also seems kind of messy here. Shouldn't this be set when the individual report is requested then?

It does require a restart and it's fine. This is a simple temporary fix for something that will happen a handful of times before the VID Model Repo comes on line, which is the proper solution to this problem.

[stevenwarejones]

Reviewed 1 of 2 files at r4.
Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @kungfucraig and @SanjayVas)

---

src/main/kotlin/org/wfanet/measurement/reporting/deploy/v2/common/ReportingApiServerFlags.kt line 58 at r4 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

It does require a restart and it's fine. This is a simple temporary fix for something that will happen a handful of times before the VID Model Repo comes on line, which is the proper solution to this problem.

can you add TODO to remove this once the VID Model repo comes online?

I'm a little confused how the Model Repo will address this same issue.

[kungfucraig]

Reviewable status: 3 of 6 files reviewed, 1 unresolved discussion (waiting on @SanjayVas and @stevenwarejones)

---

src/main/kotlin/org/wfanet/measurement/reporting/deploy/v2/common/ReportingApiServerFlags.kt line 58 at r4 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

can you add TODO to remove this once the VID Model repo comes online?

I'm a little confused how the Model Repo will address this same issue.

Added the TODO.

Happy to discuss how this gets addressed long-term offline.

[stevenwarejones]

Reviewed 1 of 4 files at r1, 1 of 1 files at r3, 1 of 2 files at r4, 1 of 2 files at r5, all commit messages.
Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @SanjayVas)

---

src/main/kotlin/org/wfanet/measurement/reporting/service/api/v2alpha/MetricsService.kt line 545 at r5 (raw file):

            }
        }
        // TODO(@jojijac0b): Complete support for VID Model Line

@jojijac0b - FYI

[stevenwarejones]

Reviewable status: 5 of 6 files reviewed, all discussions resolved (waiting on @SanjayVas)

[kungfucraig]

Reviewed 1 of 4 files at r1, 1 of 1 files at r3, 2 of 2 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @SanjayVas)

[stevenwarejones]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @SanjayVas)

[kungfucraig]

Reviewed 3 of 3 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @SanjayVas)