feat: Extend last report time interval to end of report (#1869)

feat: The last report time interval covers the end of the report This adds a time interval that covers the end of the report even if the look back window goes past the report_end. This additional time interval just cuts off at report_end. Currently, if the look back window goes past the report_end, there is no time interval that partially covers it.
world-federation-of-advertisers · Oct 22, 2024 · 1244232 · 1244232
1 parent 0d254e0
commit 1244232
Show file tree

Hide file tree

Showing 4 changed files with 86 additions and 53 deletions.
diff --git a/src/main/kotlin/org/wfanet/measurement/reporting/service/api/v2alpha/ReportsService.kt b/src/main/kotlin/org/wfanet/measurement/reporting/service/api/v2alpha/ReportsService.kt
@@ -784,14 +784,6 @@ class ReportsService(
               internalMetricCalculationSpec.details.metricFrequencySpec,
               trailingWindow,
             )
-          grpcRequire(generatedTimeIntervals.isNotEmpty()) {
-            "No time intervals can be generated from the combination of the reporting_interval and metric_calculation_spec ${
-              MetricCalculationSpecKey(
-                internalMetricCalculationSpec.cmmsMeasurementConsumerId,
-                internalMetricCalculationSpec.externalMetricCalculationSpecId,
-              ).toName()
-            }"
-          }
           generatedTimeIntervals
         }
       }
@@ -982,7 +974,7 @@ private fun validateTime(report: Report) {
         }
         .isBefore(reportEnd)
     ) {
-      "report_end be after report_start."
+      "report_end is not after report_start."
     }
   } else {
     failGrpc { "time is not set." }
@@ -1131,10 +1123,6 @@ private fun generateTimeIntervals(
       if (ChronoUnit.SECONDS.between(reportStartTemporal, nextTimeIntervalEndTemporal) > 0L) {
         val nextTimeIntervalEndTimestamp = nextTimeIntervalEndTemporal.toTimestamp()
 
-        if (Timestamps.compare(nextTimeIntervalEndTimestamp, reportEndTimestamp) > 0) {
-          break
-        }
-
         val nextTimeIntervalStartTimestamp: Timestamp =
           if (isWindowReportStart) {
             reportStartTimestamp
@@ -1153,6 +1141,18 @@ private fun generateTimeIntervals(
             }
           }
 
+        if (Timestamps.compare(nextTimeIntervalEndTimestamp, reportEndTimestamp) > 0) {
+          if (Timestamps.compare(nextTimeIntervalStartTimestamp, reportEndTimestamp) < 0) {
+            add(
+              interval {
+                startTime = nextTimeIntervalStartTimestamp
+                endTime = reportEndTimestamp
+              }
+            )
+          }
+          break
+        }
+
         add(
           interval {
             startTime = nextTimeIntervalStartTimestamp

diff --git a/src/main/proto/wfa/measurement/reporting/v2alpha/metric_calculation_spec.proto b/src/main/proto/wfa/measurement/reporting/v2alpha/metric_calculation_spec.proto
@@ -98,16 +98,21 @@ message MetricCalculationSpec {
       // A time interval is created for each day.
       Daily daily = 1;
       // A time interval is created for each week, starting on the first
-      // occurrence of the `day_of_week`. If `report_start` date is Monday and
+      // occurrence of the `day_of_week`. If `report_start` date is a Monday and
       // `day_of_week` is Tuesday, the first time interval will be created for
-      // the following day.
+      // the time period between that Monday and Tuesday, which will be 1 day
+      // long. The last time interval created will cover up to
+      // `reporting_interval.report_end` in a `Report` instead of the
+      // `day_of_week` specified.
       Weekly weekly = 2;
       // A time interval is created for each month, starting on the first
       // occurrence of the `day_of_month`. If `report_start` date is the 1st of
       // the month and `day_of_month` is 14, the first time interval will be
-      // created starting with the 14th of that month. If 'day_of_month` is
+      // from the 1st to the 14th of the month. If 'day_of_month` is
       // larger than the amount of days the month has, a time interval will be
-      // created for the end of the month.
+      // created for the end of the month. The last time interval created will
+      // cover up to `reporting_interval.report_end` in a `Report` instead of
+      // the `day_of_month` specified.
       Monthly monthly = 3;
     }
   }
@@ -141,7 +146,8 @@ message MetricCalculationSpec {
   }
 
   // The `TrailingWindow` for this `MetricCalculationSpec`. The beginning of the
-  // time covered is bounded by `ReportingInterval.report_start`.
+  // time covered is bounded by `ReportingInterval.report_start` and the end of
+  // the time covered is bounded by `ReportingInterval.report_end`.
   //
   // If set, but `metric_frequency_spec` is not, an error will be thrown.
   // If set and `metric_frequency_spec` is also set, the combination of this

diff --git a/src/main/proto/wfa/measurement/reporting/v2alpha/report.proto b/src/main/proto/wfa/measurement/reporting/v2alpha/report.proto
@@ -84,7 +84,7 @@ message Report {
   ];
 
   // The interval over which Metrics are generated. This interval is
-  // closed on the left hand side and open on the right hand side.
+  // closed on both sides.
   message ReportingInterval {
     // The date and time of the start of the report. The year,
     // month, day, and time_offset are all required.
@@ -99,7 +99,7 @@ message Report {
     ];
 
     // The end date of the report. The time is considered to be the same as
-    // that specified in the report start. This side of the interval is open.
+    // that specified in the report start. This side of the interval is closed.
     google.type.Date report_end = 2 [
       (google.api.field_behavior) = REQUIRED,
       (google.api.field_behavior) = IMMUTABLE
@@ -117,7 +117,8 @@ message Report {
     // A single time interval specifying the start and end time of the `Report`.
     // If this is specified, then for each `MetricCalculationSpec`,
     // `metric_frequency_spec` and `trailing_window` will be used to divide the
-    // interval. Any interval created will have a lower bound of report_start.
+    // interval. Any interval created will have a lower bound of `report_start`
+    // and an upper bound of `report_end`.
     // If no `metric_frequency_spec` is specified then the entire interval is
     // reported on.
     ReportingInterval reporting_interval = 10;

diff --git a/src/test/kotlin/org/wfanet/measurement/reporting/service/api/v2alpha/ReportsServiceTest.kt b/src/test/kotlin/org/wfanet/measurement/reporting/service/api/v2alpha/ReportsServiceTest.kt
@@ -747,6 +747,16 @@ class ReportsServiceTest {
             }
           }
         )
+        add(
+          interval {
+            startTime = timestamp {
+              seconds = 1705392000 // January 16, 2024 at 12:00 AM, America/Los_Angeles
+            }
+            endTime = timestamp {
+              seconds = 1705564800 // January 18, 2024 at 12:00 AM, America/Los_Angeles
+            }
+          }
+        )
       }
 
       val frequencySpec =
@@ -868,6 +878,16 @@ class ReportsServiceTest {
             }
           }
         )
+        add(
+          interval {
+            startTime = timestamp {
+              seconds = 1709280000 // March 1, 2024 at 12:00 AM, America/Los_Angeles
+            }
+            endTime = timestamp {
+              seconds = 1709625600 // March 5, 2024 at 12:00 AM, America/Los_Angeles
+            }
+          }
+        )
       }
 
       val frequencySpec =
@@ -997,6 +1017,16 @@ class ReportsServiceTest {
           }
         }
       )
+      add(
+        interval {
+          startTime = timestamp {
+            seconds = 1709280000 // March 1, 2024 at 12:00 AM, America/Los_Angeles
+          }
+          endTime = timestamp {
+            seconds = 1709625600 // March 5, 2024 at 12:00 AM, America/Los_Angeles
+          }
+        }
+      )
     }
 
     val frequencySpec =
@@ -1117,6 +1147,16 @@ class ReportsServiceTest {
             }
           }
         )
+        add(
+          interval {
+            startTime = timestamp {
+              seconds = 1704096000 // January 1, 2024 at 12:00 AM, America/Los_Angeles
+            }
+            endTime = timestamp {
+              seconds = 1709625600 // March 5, 2024 at 12:00 AM, America/Los_Angeles
+            }
+          }
+        )
       }
 
       val frequencySpec =
@@ -1165,6 +1205,16 @@ class ReportsServiceTest {
           }
         }
       )
+      add(
+        interval {
+          startTime = timestamp {
+            seconds = 1704096000 // January 1, 2024 at 12:00 AM, America/Los_Angeles
+          }
+          endTime = timestamp {
+            seconds = 1705046400 // January 12, 2024 at 12:00 AM, America/Los_Angeles
+          }
+        }
+      )
     }
 
     val frequencySpec =
@@ -2828,31 +2878,7 @@ class ReportsServiceTest {
   }
 
   @Test
-  fun `createReport throws INVALID_ARGUMENT when reporting interval results in 0 time intervals`() {
-    runBlocking {
-      whenever(internalMetricCalculationSpecsMock.batchGetMetricCalculationSpecs(any()))
-        .thenReturn(
-          batchGetMetricCalculationSpecsResponse {
-            metricCalculationSpecs +=
-              INTERNAL_REACH_METRIC_CALCULATION_SPEC.copy {
-                details =
-                  INTERNAL_REACH_METRIC_CALCULATION_SPEC.details.copy {
-                    metricFrequencySpec =
-                      MetricCalculationSpecKt.metricFrequencySpec {
-                        monthly =
-                          MetricCalculationSpecKt.MetricFrequencySpecKt.monthly { dayOfMonth = 15 }
-                      }
-                    trailingWindow =
-                      MetricCalculationSpecKt.trailingWindow {
-                        count = 1
-                        increment = MetricCalculationSpec.TrailingWindow.Increment.WEEK
-                      }
-                  }
-              }
-          }
-        )
-    }
-
+  fun `createReport throws INVALID_ARGUMENT when report_start after report_end`() {
     val request = createReportRequest {
       parent = MEASUREMENT_CONSUMER_KEYS.first().toName()
       report =
@@ -2864,15 +2890,15 @@ class ReportsServiceTest {
           reportingInterval =
             ReportKt.reportingInterval {
               reportStart = dateTime {
-                year = 2023
+                year = 3000
                 month = 1
                 day = 1
-                utcOffset = duration { seconds = 60 * 60 * -8 }
+                timeZone = timeZone { id = "America/Los_Angeles" }
               }
               reportEnd = date {
-                year = 2023
+                year = 2024
                 month = 1
-                day = 5
+                day = 1
               }
             }
         }
@@ -2886,11 +2912,11 @@ class ReportsServiceTest {
         }
       }
     assertThat(exception.status.code).isEqualTo(Status.Code.INVALID_ARGUMENT)
-    assertThat(exception.message).contains("No time intervals")
+    assertThat(exception.message).contains("report_end")
   }
 
   @Test
-  fun `createReport throws INVALID_ARGUMENT when report_start after report_end`() {
+  fun `createReport throws INVALID_ARGUMENT when report_start same as report_end`() {
     val request = createReportRequest {
       parent = MEASUREMENT_CONSUMER_KEYS.first().toName()
       report =
@@ -2902,7 +2928,7 @@ class ReportsServiceTest {
           reportingInterval =
             ReportKt.reportingInterval {
               reportStart = dateTime {
-                year = 3000
+                year = 2024
                 month = 1
                 day = 1
                 timeZone = timeZone { id = "America/Los_Angeles" }