Add Support for different Privacy Params for Single and Multiple Data Providers

[tristanvuong2021]

No description provided.

[wfa-reviewable]

This change is 

[kungfucraig]

Reviewable status: 0 of 31 files reviewed, 1 unresolved discussion (waiting on @tristanvuong2021)

---

src/main/proto/wfa/measurement/reporting/v2alpha/metric.proto line 54 at r1 (raw file):

  }

  message Params {

Maybe just have a separate Params structure for R/F?

The stray field is weird.

[tristanvuong2021]

Reviewable status: 0 of 31 files reviewed, 1 unresolved discussion (waiting on @kungfucraig)

---

src/main/proto/wfa/measurement/reporting/v2alpha/metric.proto line 54 at r1 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

Maybe just have a separate Params structure for R/F?

The stray field is weird.

Done.

[SanjayVas]

Reviewable status: 0 of 31 files reviewed, 4 unresolved discussions (waiting on @kungfucraig, @riemanli, and @tristanvuong2021)

---

src/main/proto/wfa/measurement/reporting/v2alpha/metric.proto line 54 at r3 (raw file):

  }

  message Params {

Mark the body fields of the new Params types as REQUIRED.

---

src/main/proto/wfa/measurement/reporting/v2alpha/metric.proto line 59 at r3 (raw file):

  }

  message FrequencyParams {

nit: not the biggest fan of this name, but can't come up with anything that's objectively better

Maybe call the other one ScalarParams and this one ScalarAndHistogramParams, since the first is used for all metrics with a single scalar result?

Code quote:

FrequencyParams

---

src/main/proto/wfa/measurement/reporting/v2alpha/metric.proto line 60 at r3 (raw file):

  message FrequencyParams {
    DifferentialPrivacyParams privacy_params = 1;

Might as well be explicit here.

Suggestion:

reach_privacy_params

[kungfucraig]

Reviewable status: 0 of 31 files reviewed, 4 unresolved discussions (waiting on @riemanli, @SanjayVas, and @tristanvuong2021)

---

src/main/proto/wfa/measurement/reporting/v2alpha/metric.proto line 59 at r3 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

nit: not the biggest fan of this name, but can't come up with anything that's objectively better

Maybe call the other one ScalarParams and this one ScalarAndHistogramParams, since the first is used for all metrics with a single scalar result?

I really don't like "FrequencyParams."

Maybe: SamplingAndPrivacyParams and SamplingAndPrivacyParamsForReachAndFrequency?

Won't quite win a camel case award, but getting there.

[tristanvuong2021]

Reviewable status: 0 of 31 files reviewed, 4 unresolved discussions (waiting on @kungfucraig, @riemanli, and @SanjayVas)

---

src/main/proto/wfa/measurement/reporting/v2alpha/metric.proto line 59 at r3 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

I really don't like "FrequencyParams."

Maybe: SamplingAndPrivacyParams and SamplingAndPrivacyParamsForReachAndFrequency?

Won't quite win a camel case award, but getting there.

I went with your naming suggestion

---

src/main/proto/wfa/measurement/reporting/v2alpha/metric.proto line 60 at r3 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Might as well be explicit here.

Done.

[kungfucraig]

Public API definitions look good. Will defer to others on the rest.

Reviewable status: 0 of 31 files reviewed, 2 unresolved discussions (waiting on @riemanli, @SanjayVas, and @tristanvuong2021)

---

src/main/proto/wfa/measurement/reporting/v2alpha/metric.proto line 59 at r3 (raw file):

Previously, tristanvuong2021 (Tristan Vuong) wrote…

I went with your naming suggestion

lgtm

I will defer to Sanjay on the rest of the PR. If you end up needing a final approval from me at the end let me know.

[SanjayVas]

Taking this in passes since it's a big PR.

Reviewed 3 of 31 files at r1, 4 of 21 files at r4.
Reviewable status: 7 of 31 files reviewed, 5 unresolved discussions (waiting on @riemanli and @tristanvuong2021)

---

src/main/proto/wfa/measurement/reporting/v2alpha/metric.proto line 59 at r3 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

lgtm

I will defer to Sanjay on the rest of the PR. If you end up needing a final approval from me at the end let me know.

Just one nit on the new names: avoid "For" and use a prefix instead. ReachAndFrequencySamplingAndPrivacyParams.

---

src/main/resources/reporting/postgres/add-additional-metric-spec-columns-to-metrics.sql line 40 at r5 (raw file):

  DifferentialPrivacyEpsilon DOUBLE PRECISION NOT NULL,

Err, why does this changeset just start with a list of columns?

---

src/test/kotlin/org/wfanet/measurement/reporting/service/api/v2alpha/MetricCalculationSpecsServiceTest.kt line 1440 at r5 (raw file):

    private val SECRET_FILES_PATH: Path = run {
      val runfiles =

Continue to use the (deprecated) common-jvm functions for Runfiles until #1530 is fixed.

---

src/test/kotlin/org/wfanet/measurement/reporting/service/api/v2alpha/MetricSpecDefaultsTest.kt line 215 at r5 (raw file):

  @Test
  fun `buildMetricSpec builds reach spec when fields filled in multiple and single edp fields`() {

nit: the tests like this which are basically checking that withDefaults does not override specified fields should have test method names that more clearly indicate that.

---

src/test/kotlin/org/wfanet/measurement/reporting/service/api/v2alpha/MetricSpecDefaultsTest.kt line 1500 at r5 (raw file):

    // Metric Specs

    private val OLD_EMPTY_REACH_METRIC_SPEC: MetricSpec = metricSpec {

nit: LEGACY for the old messages that still need to be handled, and no prefix for the new ones

Suggestion:

LEGACY_EMPTY

[tristanvuong2021]

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

---

src/main/resources/reporting/postgres/add-is-single-data-provider-column-to-measurements.sql line 40 at r6 (raw file):

Previously, tristanvuong2021 (Tristan Vuong) wrote…

1. It is not used in any queries, but because I set details later when measurements complete, it is easier to not put it details.
2. Is it because we might have other cases besides single and multiple Data Providers in the future.

Used your suggestions.

[SanjayVas]

Reviewed 1 of 14 files at r6, 4 of 20 files at r7, all commit messages.
Reviewable status: 7 of 35 files reviewed, 6 unresolved discussions (waiting on @riemanli and @tristanvuong2021)

---

src/main/kotlin/org/wfanet/measurement/reporting/service/api/v2alpha/MetricSpecDefaults.kt line 288 at r7 (raw file):

 */
private fun MetricSpec.ReachParams.withDefaults(
  metricSpec: MetricSpec,

nit: instead of passing in the whole parent of ReachParams, maybe pass in a nullable deprecatedVidSamplingInterval for the deprecated field value?

---

src/main/kotlin/org/wfanet/measurement/reporting/service/api/v2alpha/MetricSpecDefaults.kt line 294 at r7 (raw file):

  return copy {
    if (
      multipleDataProviderParams.hasPrivacyParams() &&

nit: I think might be clearer to do the validation separately up front in this case rather than intermingling it with the protobuf building. This will also let you have more specific error messages.

---

src/main/proto/wfa/measurement/reporting/v2alpha/metric.proto line 47 at r7 (raw file):

  message VidSamplingInterval {
    // The start of the sampling interval in [0, 1)
    optional float start = 1;

Is there a behavioral difference between 0 and unset here? If so, it should be documented here. If not, then do not use the optional keyword.

Code quote:

optional

[tristanvuong2021]

Reviewable status: 6 of 35 files reviewed, 4 unresolved discussions (waiting on @riemanli and @SanjayVas)

---

src/main/proto/wfa/measurement/reporting/v2alpha/metric.proto line 47 at r7 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Is there a behavioral difference between 0 and unset here? If so, it should be documented here. If not, then do not use the optional keyword.

Done.

[SanjayVas]

Reviewed 2 of 20 files at r7, 1 of 2 files at r8.
Reviewable status: 9 of 35 files reviewed, 7 unresolved discussions (waiting on @riemanli and @tristanvuong2021)

---

src/main/kotlin/org/wfanet/measurement/reporting/service/api/v2alpha/MetricSpecDefaults.kt line 677 at r8 (raw file):

    val maxStart = NUM_BUCKETS - (source.randomStart.width * NUM_BUCKETS).toInt()
    // The `- 1` is in case the rounding from source.randomStart.width * NUM_BUCKETS rounds down.
    // This

nit: wrapping

---

src/main/proto/wfa/measurement/config/reporting/metric_spec_config.proto line 34 at r8 (raw file):

  // Computer Science, 9(3-4), pp.211-407."
  message DifferentialPrivacyParams {
    optional double epsilon = 1;

Nothing should be optional in the config. All fields must be set, meaning unset is equivalent to setting to 0.

We don't need backwards compatibility in the config like we do for the API message. The incompatible change just needs to be made clear in the release notes (and therefore in the PR description).

---

src/main/proto/wfa/measurement/reporting/v2alpha/metric.proto line 47 at r8 (raw file):

  //
  // If any of the fields are not specified in a request message, the service
  // implementation will choose values.

Just to confirm, we want to allow you to e.g. specify width but not start to get the default start but the specified width? Does that actually make sense?

Code quote:

  // If any of the fields are not specified in a request message, the service
  // implementation will choose values.

---

src/main/proto/wfa/measurement/reporting/v2alpha/metric.proto line 54 at r8 (raw file):

    //
    // start + width cannot be larger than 1
    optional float width = 2;

To clarify, this means that 0 is a valid width. Is that correct?

Code quote:

optional 

[kungfucraig]

Reviewable status: 9 of 35 files reviewed, 7 unresolved discussions (waiting on @riemanli, @SanjayVas, and @tristanvuong2021)

---

src/main/proto/wfa/measurement/reporting/v2alpha/metric.proto line 54 at r8 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

To clarify, this means that 0 is a valid width. Is that correct?

We should have: 0 < width <= 1

We don't want an empty interval.

[tristanvuong2021]

Reviewable status: 7 of 35 files reviewed, 6 unresolved discussions (waiting on @riemanli and @SanjayVas)

---

src/main/proto/wfa/measurement/config/reporting/metric_spec_config.proto line 34 at r8 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Nothing should be optional in the config. All fields must be set, meaning unset is equivalent to setting to 0.

We don't need backwards compatibility in the config like we do for the API message. The incompatible change just needs to be made clear in the release notes (and therefore in the PR description).

Done.

---

src/main/proto/wfa/measurement/reporting/v2alpha/metric.proto line 47 at r8 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Just to confirm, we want to allow you to e.g. specify width but not start to get the default start but the specified width? Does that actually make sense?

Changed it so it uses defaults for both or none.

---

src/main/proto/wfa/measurement/reporting/v2alpha/metric.proto line 54 at r8 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

We should have: 0 < width <= 1

We don't want an empty interval.

I added the comment to clarify it, but I realized I am not sure about the valid interval for epsilon and delta. We do have a check in the Kingdom Measurements service, but we don't document it in the API proto.

[SanjayVas]

Reviewed 2 of 4 files at r9.
Reviewable status: 9 of 35 files reviewed, 3 unresolved discussions (waiting on @riemanli and @tristanvuong2021)

---

src/main/proto/wfa/measurement/reporting/v2alpha/metric.proto line 47 at r8 (raw file):

Previously, tristanvuong2021 (Tristan Vuong) wrote…

Changed it so it uses defaults for both or none.

The clearer way to do that is to mark width as required and have the VidSamplingInterval fields be optional. This way you can just check the presence of the VidSamplingInterval field.

---

src/main/proto/wfa/measurement/reporting/v2alpha/metric.proto line 54 at r8 (raw file):

Previously, tristanvuong2021 (Tristan Vuong) wrote…

I added the comment to clarify it, but I realized I am not sure about the valid interval for epsilon and delta. We do have a check in the Kingdom Measurements service, but we don't document it in the API proto.

To reiterate my statement: if this field has the optional keyword, it is implying that 0 is a valid value. The way to indicate that 0 is not valid is to specify that the field behavior is REQUIRED.

[tristanvuong2021]

Reviewable status: 8 of 35 files reviewed, 1 unresolved discussion (waiting on @riemanli and @SanjayVas)

---

src/main/proto/wfa/measurement/reporting/v2alpha/metric.proto line 47 at r8 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

The clearer way to do that is to mark width as required and have the VidSamplingInterval fields be optional. This way you can just check the presence of the VidSamplingInterval field.

Done.

---

src/main/proto/wfa/measurement/reporting/v2alpha/metric.proto line 54 at r8 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

To reiterate my statement: if this field has the optional keyword, it is implying that 0 is a valid value. The way to indicate that 0 is not valid is to specify that the field behavior is REQUIRED.

That is not what i wanted to confirm here. I wanted to confirm if it is still the case that delta can be 0, but epsilon can't. That is what we have in code, but we don't document it.

[SanjayVas]

Reviewed 1 of 3 files at r10, all commit messages.
Reviewable status: 9 of 35 files reviewed, 5 unresolved discussions (waiting on @riemanli and @tristanvuong2021)

---

src/main/kotlin/org/wfanet/measurement/reporting/service/api/v2alpha/MetricSpecDefaults.kt line 25 at r10 (raw file):

import org.wfanet.measurement.reporting.v2alpha.copy

private const val NUM_BUCKETS = 10000

Where does this come from and what does it mean? Document it.

---

src/main/kotlin/org/wfanet/measurement/reporting/service/api/v2alpha/MetricSpecDefaults.kt line 293 at r10 (raw file):

  secureRandom: Random,
): MetricSpec.ReachParams {
  if (this.hasMultipleDataProviderParams() && this.hasSingleDataProviderParams()) {

nit: personally I think this would be clearer as

if (hasMultipleDataProviderParams() || hasSingleDataProviderParams()) {
  if (hasMultipleDataProviderParams() != hasSingleDataProviderParams()) {
    // Throw exception for not having both multi and single set ...
  }
  // Validate the subfields of single and multi ...
} else {
  if (!hasPrivacyParams()) {
    // Throw exception for not having any privacy params set ...
  }
  // Validate the subfields of privacy params.
}

---

src/main/kotlin/org/wfanet/measurement/reporting/service/api/v2alpha/MetricSpecDefaults.kt line 355 at r10 (raw file):

        }
    } else {
      val source = this

I believe you need this line outside of the copy lambda.

---

src/main/kotlin/org/wfanet/measurement/reporting/service/api/v2alpha/MetricSpecDefaults.kt line 371 at r10 (raw file):

          vidSamplingInterval.validate()
        }
      clearPrivacyParams()

Don't you also need to set singleDataProviderParams to be the same as multipleDataProviderParams in this case?

---

src/main/proto/wfa/measurement/reporting/v2alpha/metric.proto line 54 at r8 (raw file):

Previously, tristanvuong2021 (Tristan Vuong) wrote…

That is not what i wanted to confirm here. I wanted to confirm if it is still the case that delta can be 0, but epsilon can't. That is what we have in code, but we don't document it.

Privacy params are a separate discussion. Marking this resolvedw.r.t. sampling interval, as width now has field_behavior=REQUIRED and no optional keyword.

---

src/main/proto/wfa/measurement/reporting/v2alpha/metric.proto line 47 at r10 (raw file):

  //
  // If both start and width are not specified, the service implementation will
  // choose values for both start and width.

This comment is inaccurate. Any comment about the defaults should go on the fields that use this message type, and are based on whether the field itself is set.

Code quote:

  //
  // If both start and width are not specified, the service implementation will
  // choose values for both start and width.

[tristanvuong2021]

Reviewable status: 8 of 35 files reviewed, 4 unresolved discussions (waiting on @riemanli and @SanjayVas)

---

src/main/kotlin/org/wfanet/measurement/reporting/service/api/v2alpha/MetricSpecDefaults.kt line 25 at r10 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Where does this come from and what does it mean? Document it.

This is to help with the math. I just removed it since it is only ever used in one function.

---

src/main/kotlin/org/wfanet/measurement/reporting/service/api/v2alpha/MetricSpecDefaults.kt line 355 at r10 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

I believe you need this line outside of the copy lambda.

It doesn't change how it works, but I moved it for clarity.

---

src/main/kotlin/org/wfanet/measurement/reporting/service/api/v2alpha/MetricSpecDefaults.kt line 371 at r10 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Don't you also need to set singleDataProviderParams to be the same as multipleDataProviderParams in this case?

Right now, I have it set up to only set multiple and when creating the measurements, if only multiple is available, it uses that for single edp as well. And when retrieving the metric spec, if only multiple is set, it sets the legacy privacy_params.

---

src/main/proto/wfa/measurement/reporting/v2alpha/metric.proto line 47 at r10 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

This comment is inaccurate. Any comment about the defaults should go on the fields that use this message type, and are based on whether the field itself is set.

Done.

[SanjayVas]

Reviewable status: 8 of 35 files reviewed, 2 unresolved discussions (waiting on @riemanli and @tristanvuong2021)

---

src/main/kotlin/org/wfanet/measurement/reporting/service/api/v2alpha/MetricSpecDefaults.kt line 371 at r10 (raw file):

Previously, tristanvuong2021 (Tristan Vuong) wrote…

Right now, I have it set up to only set multiple and when creating the measurements, if only multiple is available, it uses that for single edp as well. And when retrieving the metric spec, if only multiple is set, it sets the legacy privacy_params.

So this is to persist the fact that a legacy request set the legacy params? I'm not entirely certain that's necessary, but I guess it's fine.

[SanjayVas]

Reviewable status: 8 of 35 files reviewed, 2 unresolved discussions (waiting on @riemanli and @tristanvuong2021)

---

src/main/proto/wfa/measurement/reporting/v2alpha/metric.proto line 58 at r11 (raw file):

    // If both start and width are not specified, the service implementation
    // will choose values for both start and width.

This is not quite accurate. Again, it's based upon the field presence. If the field is present, then all of the subfield field behavior annotations apply. Therefore it's sufficient to say "If not specified, the service implementation will choose a default value."

Also every field comment should start with a summary fragment describing the field.

Code quote:

    // If both start and width are not specified, the service implementation
    // will choose values for both start and width.

[tristanvuong2021]

Reviewable status: 8 of 35 files reviewed, 1 unresolved discussion (waiting on @riemanli and @SanjayVas)

---

src/main/proto/wfa/measurement/reporting/v2alpha/metric.proto line 58 at r11 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

This is not quite accurate. Again, it's based upon the field presence. If the field is present, then all of the subfield field behavior annotations apply. Therefore it's sufficient to say "If not specified, the service implementation will choose a default value."

Also every field comment should start with a summary fragment describing the field.

Done.

[SanjayVas]

Reviewed 1 of 31 files at r1, 2 of 9 files at r2, 1 of 17 files at r3, 4 of 21 files at r4, 2 of 14 files at r6, 8 of 20 files at r7, 1 of 2 files at r11, 1 of 1 files at r12, all commit messages.
Reviewable status: 28 of 35 files reviewed, 2 unresolved discussions (waiting on @riemanli and @tristanvuong2021)

---

src/main/kotlin/org/wfanet/measurement/measurementconsumer/stats/MeasurementStatistics.kt line 93 at r12 (raw file):

 * reach_result +/- 1.96 * reach_std.
 */
private const val REACH_THRESHLEGACY_CONSTANT_FOR_RELATIVE_FREQUENCY_VARIANCE = 1.96

I think your find/replace was a little aggressive here.

---

src/main/proto/wfa/measurement/internal/reporting/v2/metric.proto line 47 at r12 (raw file):

  message ReachParams {
    SamplingAndPrivacyParams multiple_data_provider_params = 2;

Mark the previously used field numbers as reserved. See https://protobuf.dev/programming-guides/proto3/#fieldreserved

[tristanvuong2021]

Reviewable status: 26 of 35 files reviewed, 2 unresolved discussions (waiting on @riemanli and @SanjayVas)

---

src/main/proto/wfa/measurement/internal/reporting/v2/metric.proto line 47 at r12 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Mark the previously used field numbers as reserved. See https://protobuf.dev/programming-guides/proto3/#fieldreserved

Done.

---

src/main/kotlin/org/wfanet/measurement/measurementconsumer/stats/MeasurementStatistics.kt line 93 at r12 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

I think your find/replace was a little aggressive here.

Done.

[SanjayVas]

Reviewed 2 of 14 files at r6, 4 of 20 files at r7, 1 of 3 files at r10, 2 of 2 files at r13, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @riemanli and @tristanvuong2021)

---

src/main/proto/wfa/measurement/internal/reporting/v2/metric.proto line 52 at r13 (raw file):

  }
  message ReachAndFrequencyParams {
    reserved = 1;

nit: you can technicaly do this as reserved 1,2;