[config_parser] Fix off-by-one error in calculation of num_index_points
In https://fxrev.dev/485978, the config parser set the num_index_points
field of a ReportDefinition to the width of the range of valid integer
values when the integer range is small. However, we want to include both
endpoints of the integer range as index points, so the value of
num_index_points was off by 1 in that CL. This CL fixes that error +
some golint errors.
Change-Id: I3be8caa0d7b9f4c1f03adff049368ede377dc16a
Reviewed-on: https://fuchsia-review.googlesource.com/c/cobalt/+/486957
Reviewed-by: Jared Weinstein <jaredweinstein@google.com>
Commit-Queue: Laura Peskin <pesk@google.com>
diff --git a/src/bin/config_parser/src/config_parser/populate_privacy_params_test.go b/src/bin/config_parser/src/config_parser/populate_privacy_params_test.go
index d1b7fc1..dbbe5f6 100644
--- a/src/bin/config_parser/src/config_parser/populate_privacy_params_test.go
+++ b/src/bin/config_parser/src/config_parser/populate_privacy_params_test.go
@@ -211,11 +211,11 @@
// The sparsity is |eventCodeBufferMax| = 5.
// The best-match record has key (1.0, 10000, 10).
{args{&occurrenceMetric, &highPrivacyFleetwideOccurrenceCountsReport}, true,
- expectedParams{0.019537480548024178, uint32(maxValue)}},
+ expectedParams{0.019537480548024178, uint32(maxValue + 1)}},
// The sparsity is |eventCodeBufferMax| * |numLinearBuckets| = 10.
// The best-match record has key (1.0, 10000, 10).
{args{&integerMetric, &highPrivacyFleetwideHistogramsReport}, true,
- expectedParams{0.019537480548024178, uint32(maxCount)}},
+ expectedParams{0.019537480548024178, uint32(maxCount + 1)}},
}
for _, test := range tests {
err := populateParamsForReport(calc, test.input.metric, test.input.report)
diff --git a/src/bin/config_parser/src/privacy/privacy_encoding_params.go b/src/bin/config_parser/src/privacy/privacy_encoding_params.go
index df864c1..80d038d 100644
--- a/src/bin/config_parser/src/privacy/privacy_encoding_params.go
+++ b/src/bin/config_parser/src/privacy/privacy_encoding_params.go
@@ -206,10 +206,11 @@
sort.Slice(*vals, func(i, j int) bool { return (*vals)[i] < (*vals)[j] })
}
-// Given a |metric| and |report|, looks up the corresponding PrivacyEncodingParams from |calc|'s
-// paramMap. If |report| has an integer range size which is smaller than the number of index points
-// computed by |calc|, this function uses that integer range size as the NumIndexPoints field of the
-// returned PrivacyEncodingParams in order to avoid incurring unnecessary rounding error.
+// GetPrivacyEncodingParamsForReport looks up the corresponding PrivacyEncodingParams from |calc|'s
+// paramMap, given a |metric| and |report|. If |report| has an integer range size which is smaller
+// than the number of index points computed by |calc|, this function uses that integer range size as
+// the NumIndexPoints field of the returned PrivacyEncodingParams in order to avoid incurring
+// unnecessary rounding error.
//
// If paramMap does not have a key which exactly matches the values drawn from |metric|, |report|,
// and |calc.constants|, then parameters are returned for the closest key which provides at least as
@@ -225,7 +226,7 @@
return params, err
}
- range_size, err := getIntegerRangeSizeForReport(report)
+ rangeSize, err := getIntegerRangeSizeForReport(report)
if err != nil {
return params, err
}
@@ -235,8 +236,8 @@
return params, err
}
- if range_size < uint64(params.NumIndexPoints) {
- params.NumIndexPoints = uint32(range_size)
+ if rangeSize < uint64(params.NumIndexPoints) {
+ params.NumIndexPoints = uint32(rangeSize)
}
return params, err
@@ -262,51 +263,52 @@
}
// Returns the number of valid integer values for |report|. For FleetwideOccurrenceCounts,
-// UniqueDeviceNumericStats, and HourlyValueNumericStats reports, this is the difference between
-// the report's MaxValue and MinValue fields. For FleetwideHistograms and StringCounts reports,
-// it is the report's MaxCount field.
+// UniqueDeviceNumericStats, and HourlyValueNumericStats reports, this is the number of integers
+// in the range [|report.MinValue|, |report.MaxValue|] including both endpoints. For
+// FleetwideHistograms and StringCounts reports it is the number of integers in the range
+// [0, |report.MaxCount|].
//
-// For UniqueDeviceCounts, UniqueDeviceHistograms, and HourlyValueHistograms reports, this notion
-// doesn't apply and the returned range size is 0.
+// For UniqueDeviceCounts, UniqueDeviceHistograms, and HourlyValueHistograms reports, all observations
+// contain the same (implicit or explicit) numeric value of 1, so the returned range size is 1.
//
// A FleetwideMeans report has two separate configured ranges: one for sum values and another for
// count values. The returned range size is the maximum of the two range sizes.
-func getIntegerRangeSizeForReport(report *config.ReportDefinition) (range_size uint64, err error) {
+func getIntegerRangeSizeForReport(report *config.ReportDefinition) (rangeSize uint64, err error) {
switch report.ReportType {
case config.ReportDefinition_FLEETWIDE_OCCURRENCE_COUNTS,
config.ReportDefinition_UNIQUE_DEVICE_NUMERIC_STATS,
config.ReportDefinition_HOURLY_VALUE_NUMERIC_STATS:
{
- size := report.MaxValue - report.MinValue
- if size >= 0 {
- range_size = uint64(size)
+ size := report.MaxValue - report.MinValue + 1
+ if size > 0 {
+ rangeSize = uint64(size)
} else {
- return range_size, fmt.Errorf("min value %d is larger than max value %d", report.MinValue, report.MaxValue)
+ return rangeSize, fmt.Errorf("min value %d is larger than max value %d", report.MinValue, report.MaxValue)
}
}
case config.ReportDefinition_FLEETWIDE_HISTOGRAMS,
config.ReportDefinition_STRING_COUNTS:
- range_size, err = report.MaxCount, nil
+ rangeSize, err = report.MaxCount+1, nil
case config.ReportDefinition_FLEETWIDE_MEANS:
{
- sum_size := report.MaxValue - report.MinValue
- if sum_size >= 0 {
- range_size = uint64(sum_size)
+ sumSize := report.MaxValue - report.MinValue + 1
+ if sumSize > 0 {
+ rangeSize = uint64(sumSize)
} else {
- return range_size, fmt.Errorf("min value %d is larger than max value %d", report.MinValue, report.MaxValue)
+ return rangeSize, fmt.Errorf("min value %d is larger than max value %d", report.MinValue, report.MaxValue)
}
- if range_size < report.MaxCount {
- range_size = report.MaxCount
+ if rangeSize < report.MaxCount {
+ rangeSize = report.MaxCount + 1
}
}
case config.ReportDefinition_UNIQUE_DEVICE_COUNTS,
config.ReportDefinition_UNIQUE_DEVICE_HISTOGRAMS,
config.ReportDefinition_HOURLY_VALUE_HISTOGRAMS:
- range_size = 0
+ rangeSize = 1
default:
- return range_size, fmt.Errorf("unsupported ReportType: %v", report.ReportType)
+ return rangeSize, fmt.Errorf("unsupported ReportType: %v", report.ReportType)
}
- return range_size, nil
+ return rangeSize, nil
}
diff --git a/src/bin/config_parser/src/privacy/privacy_encoding_params_test.go b/src/bin/config_parser/src/privacy/privacy_encoding_params_test.go
index 9db1d5d..098655a 100644
--- a/src/bin/config_parser/src/privacy/privacy_encoding_params_test.go
+++ b/src/bin/config_parser/src/privacy/privacy_encoding_params_test.go
@@ -156,16 +156,16 @@
expected uint64
}{
// Valid input:
- {&fleetwideOccurrenceCountsReport, true, uint64(maxValue - minValue)},
- {&uniqueDeviceNumericStatsReport, true, uint64(maxValue - minValue)},
- {&hourlyValueNumericStatsReport, true, uint64(maxValue - minValue)},
- {&fleetwideHistogramsReport, true, maxCount},
- {&stringCountsReport, true, maxCount},
- {&fleetwideMeansLargerSumRangeReport, true, uint64(maxValue - minValue)},
- {&fleetwideMeansLargerCountRangeReport, true, maxCount},
- {&uniqueDeviceCountsReport, true, 0},
- {&uniqueDeviceHistogramsReport, true, 0},
- {&hourlyValueHistogramsReport, true, 0},
+ {&fleetwideOccurrenceCountsReport, true, uint64(maxValue - minValue + 1)},
+ {&uniqueDeviceNumericStatsReport, true, uint64(maxValue - minValue + 1)},
+ {&hourlyValueNumericStatsReport, true, uint64(maxValue - minValue + 1)},
+ {&fleetwideHistogramsReport, true, maxCount + 1},
+ {&stringCountsReport, true, maxCount + 1},
+ {&fleetwideMeansLargerSumRangeReport, true, uint64(maxValue - minValue + 1)},
+ {&fleetwideMeansLargerCountRangeReport, true, maxCount + 1},
+ {&uniqueDeviceCountsReport, true, 1},
+ {&uniqueDeviceHistogramsReport, true, 1},
+ {&hourlyValueHistogramsReport, true, 1},
// Invalid input:
{&unsetReportTypeReport, false, 0},
}
@@ -559,6 +559,13 @@
MinValue: minValue,
MaxValue: smallMaxValue,
}
+ noMaxValueFleetwideOccurrenceCountsReport := config.ReportDefinition{
+ ReportName: "noMaxValueFleetwideOccurrenceCountsReport",
+ ReportType: config.ReportDefinition_FLEETWIDE_OCCURRENCE_COUNTS,
+ PrivacyLevel: config.ReportDefinition_LOW_PRIVACY,
+ MinValue: 0,
+ MaxValue: 0,
+ }
largeRangeSizeFleetwideOccurrenceCountsReport := config.ReportDefinition{
ReportName: "largeRangeSizeFleetwideOccurrenceCountsReport",
ReportType: config.ReportDefinition_FLEETWIDE_OCCURRENCE_COUNTS,
@@ -596,20 +603,26 @@
// Valid input:
// The target epsilon is 1.0 and the sparsity is |eventCodeBufferMax|. The best-match key is
// {1.0, 10000, 10}. The report has type UniqueDeviceCounts, so the NumIndexPoints field of
- // PrivacyEncodingParams is set to 0.
+ // PrivacyEncodingParams is set to 1.
{args{&occurrenceMetric, &highPrivacyAtLeastOnceReport}, true,
- PrivacyEncodingParams{0.019537480548024178, 0}},
+ PrivacyEncodingParams{0.019537480548024178, 1}},
// The target epsilon is 10.0 and the sparsity is 1. The best-match key is {5.0, 10000, 1}.
// The report has type UniqueDeviceCounts, so the NumIndexPoints field of PrivacyEncodingParams is
- // set to 0.
+ // set to 1.
{args{&occurrenceMetric, &lowPrivacySelectFirstReport}, true,
- PrivacyEncodingParams{0.000906200148165226, 0}},
+ PrivacyEncodingParams{0.000906200148165226, 1}},
// The target epsilon is 10.0 and the sparsity is |eventCodeBufferMax|.
// The best-match key is {5.0, 10000, 10}.
// The integer range size of the report (= |smallMaxValue|) is smaller than the number of index
- // points computed by |calc|, so the returned NumIndexPoints should be equal to |smallMaxValue|.
+ // points computed by |calc|, so the returned NumIndexPoints should be equal to |smallMaxValue| + 1.
{args{&occurrenceMetric, &smallRangeSizeFleetwideOccurrenceCountsReport}, true,
- PrivacyEncodingParams{0.0014028092846274376, uint32(smallMaxValue)}},
+ PrivacyEncodingParams{0.0014028092846274376, uint32(smallMaxValue + 1)}},
+ // The target epsilon is 10.0 and the sparsity is |eventCodeBufferMax|.
+ // The best-match key is {5.0, 10000, 10}.
+ // The integer range size of the report (= 1) is smaller than the number of index
+ // points computed by |calc|, so the returned NumIndexPoints should be equal to 1.
+ {args{&occurrenceMetric, &noMaxValueFleetwideOccurrenceCountsReport}, true,
+ PrivacyEncodingParams{0.0014028092846274376, 1}},
// The target epsilon is 10.0 and the sparsity is |eventCodeBufferMax|.
// The best-match key is {5.0, 10000, 10}.
// The integer range size of the report (= largeMaxValue) is larger than the number of index points
@@ -619,9 +632,9 @@
// The target epsilon is 10.0 and the sparsity is |eventCodeBufferMax| * |numLinearBuckets|.
// The best-match key is {5.0, 10000, 10}.
// The integer range size of the report (= |smallMaxCount|) is smaller than the number of index
- // points computed by |calc|, so the returned NumIndexPoints should be equal to |smallMaxCount|.
+ // points computed by |calc|, so the returned NumIndexPoints should be equal to |smallMaxCount + 1|.
{args{&integerMetric, &smallRangeSizeFleetwideHistogramsReport}, true,
- PrivacyEncodingParams{0.0014028092846274376, uint32(smallMaxCount)}},
+ PrivacyEncodingParams{0.0014028092846274376, uint32(smallMaxCount + 1)}},
// The target epsilon is 10.0 and the sparsity is |eventCodeBufferMax| * |numLinearBuckets|.
// The best-match key is {5.0, 10000, 10}.
// The integer range size of the report (= |largeMaxCount|) is larger than the number of index
