Add getMetric and batchGetMetric.

[riemanli]

No description provided.

[wfa-reviewable]

This change is 

[riemanli]

Current dependencies on/for this PR:

• main
  • PR v2alpha metrics service -- ListMetrics #951
    • PR Derive metric states #963
      • PR Add getMetric and batchGetMetric. #959 👈
        • PR Add SetExpressionCompiler for reporting v2alpha #968
          • PR Fix page token usage. #971

This comment was auto-generated by Graphite.

[SanjayVas]

Reviewed 1 of 2 files at r2, all commit messages.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @riemanli and @tristanvuong2021)

---

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

    return try {
      batchGetInternalMetrics(cmmsMeasurementConsumerId, listOf(externalMetricId)).first()
    } catch (e: StatusException) {

Inspect the status code and throw a StatusRuntimeException for known cases, e.g. NOT_FOUND.

---

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

        }

      // Verify proto argument of internal MetricsCoroutineImplBase::batchGetMetrics

Since you're stubbing batchGetMetrics in this test method, you can use an argument matcher in the stubbing rather than capturing/verifying the arguments separately. If the stubbed service method isn't called as expected, the other verifications will fail.

More broadly, you should generally only perform interaction verification for functions which change state. See https://abseil.io/resources/swe-book/html/ch13.html#best_practices_for_interaction_testing (Google-internal version at go/mocking#interaction-state-changes)

[riemanli]

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

---

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

Previously, SanjayVas (Sanjay Vasandani) wrote…

Inspect the status code and throw a StatusRuntimeException for known cases, e.g. NOT_FOUND.

Thanks for pointing it out! The internal metrics service hasn't been implemented yet, so I am not sure what status codes will be returned.

For other status exception catch, I added as much as I can.

---

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

Previously, SanjayVas (Sanjay Vasandani) wrote…

Since you're stubbing batchGetMetrics in this test method, you can use an argument matcher in the stubbing rather than capturing/verifying the arguments separately. If the stubbed service method isn't called as expected, the other verifications will fail.

More broadly, you should generally only perform interaction verification for functions which change state. See https://abseil.io/resources/swe-book/html/ch13.html#best_practices_for_interaction_testing (Google-internal version at go/mocking#interaction-state-changes)

Thanks for suggesting and sharing the guide. I think there are two difficulties to use argument matcher for batchGetMetrics:

1. I need to verify how many times batchGetMetrics is called for some tests. This is listed as one of the reasons to use interaction test in the guide.
2. The behavior of batchGetMetrics can't be differentiated by the input argument. For example, batchGetMetrics can return a failed metric or a pending metric given the same external metric ID.

Please let me know if there is a way to make the argument matcher work. Appreciated it!

[SanjayVas]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @riemanli and @tristanvuong2021)

---

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

I need to verify how many times batchGetMetrics is called for some tests.

Sure, though note that this is only needed in cases where e.g. you return the same result across those calls. If you return a different result on consecutive calls, then the test should fail for other reasons.

The behavior of batchGetMetrics can't be differentiated by the input argument. For example, batchGetMetrics can return a failed metric or a pending metric given the same external metric ID.

Isn't this just consecutive calls with the same matcher? That's technically what you have right now in this method, but the matcher happens to be any().

---

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

        }
      assertThat(exception.grpcStatusCode()).isEqualTo(Status.Code.INVALID_ARGUMENT)
      val expectedExceptionDescription = "Required field unspecified or invalid."

Don't assert on actual exception messages unless you need that to disambiguate between multiple causes of the same exception. We want to be able to change these messages freely. When you do need to assert on the message, do the minimum to disambiguate.

Code quote:

      val expectedExceptionDescription = "Required field unspecified or invalid."
      assertThat(exception.message).contains(expectedExceptionDescription)

[Marco-Premier]

Reviewed 1 of 2 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @riemanli, @SanjayVas, @stevenwarejones, and @tristanvuong2021)

---

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

    val principal: ReportingPrincipal = principalFromCurrentContext
    when (principal) {
      is MeasurementConsumerPrincipal -> {

Here we only checks that the MeasurementConsumer owns the metric.
What if the principal firing this API is DataProviderPrincipal for instance? Shall we handle that case and return a proper error (i.e. PERMISSION_DENIED)?

Code quote:

MeasurementConsumerPrincipal

---

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

    when (principal) {
      is MeasurementConsumerPrincipal -> {

Same as above comment

Code quote:

MeasurementConsumerPrincipal

[Marco-Premier]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @riemanli, @SanjayVas, @stevenwarejones, and @tristanvuong2021)

[stevenwarejones]

Reviewed 1 of 2 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Marco-Premier, @riemanli, @SanjayVas, and @tristanvuong2021)

---

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

Previously, riemanli (Rieman) wrote…

We don't. It's just easier to get the latest metrics in this way, without bookkeeping the mapping and maintaining the order, and I assume the table read is fairly efficient with a batch method.
Let me know if you think partial retrieval is preferred.

How large might these queries be? Maybe add a comment to look into how much of a performance improvement that might add at the least.

---

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

Previously, Marco-Premier (marcopremier) wrote…

Here we only checks that the MeasurementConsumer owns the metric.
What if the principal firing this API is DataProviderPrincipal for instance? Shall we handle that case and return a proper error (i.e. PERMISSION_DENIED)?

+1

[SanjayVas]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Marco-Premier, @riemanli, and @tristanvuong2021)

---

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

Previously, stevenwarejones (Steven Ware Jones) wrote…

+1

ReportingPrincipal is a sealed interface. Therefore this should be an "exhaustive when" such that it will not compile if new principals get added. There's no need for an else condition.

[riemanli]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Marco-Premier, @stevenwarejones, and @tristanvuong2021)

---

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

Previously, stevenwarejones (Steven Ware Jones) wrote…

How large might these queries be? Maybe add a comment to look into how much of a performance improvement that might add at the least.

Currently, we set the maximum number as 1000. It's hard to tell how much the performance can be improved without a proper test. The query itself might not take much longer, but the response will be larger proportionally. Note that the maximum number could change after we load test the system. If we are ok to wait until the test after deployment, I will add a TODO comment to indicate a potential future improvement.

---

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

Previously, SanjayVas (Sanjay Vasandani) wrote…

ReportingPrincipal is a sealed interface. Therefore this should be an "exhaustive when" such that it will not compile if new principals get added. There's no need for an else condition.

Thanks Sanjay for the explanation. Note that right now we only have MeasurementConsumerPrincipal in ReportingPrincipal. If the user uses a principal other than the principal listed in ReportingPrincipal.kt, it will be caught here.

[stevenwarejones]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Marco-Premier, @riemanli, and @tristanvuong2021)

---

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

Previously, riemanli (Rieman) wrote…

Currently, we set the maximum number as 1000. It's hard to tell how much the performance can be improved without a proper test. The query itself might not take much longer, but the response will be larger proportionally. Note that the maximum number could change after we load test the system. If we are ok to wait until the test after deployment, I will add a TODO comment to indicate a potential future improvement.

Why not add a TODO now?

---

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

Previously, riemanli (Rieman) wrote…

Thanks Sanjay for the explanation. Note that right now we only have MeasurementConsumerPrincipal in ReportingPrincipal. If the user uses a principal other than the principal listed in ReportingPrincipal.kt, it will be caught here.

you still need an else right?

[SanjayVas]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Marco-Premier, @riemanli, and @tristanvuong2021)

---

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

Previously, stevenwarejones (Steven Ware Jones) wrote…

you still need an else right?

Adding an else would make it so the when is non-exhaustive, i.e. it won't be a compiler error if a principal type is added but not handled here.

[riemanli]

Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @Marco-Premier, @stevenwarejones, and @tristanvuong2021)

---

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

Previously, stevenwarejones (Steven Ware Jones) wrote…

Why not add a TODO now?

I was checking again to see if you'd have different thoughts after my previous reply :)
Added TODOs.

[stevenwarejones]

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Marco-Premier, @riemanli, and @tristanvuong2021)

---

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

Previously, SanjayVas (Sanjay Vasandani) wrote…

Adding an else would make it so the when is non-exhaustive, i.e. it won't be a compiler error if a principal type is added but not handled here.

i guess this depends on the desired behavior. If all other principal types are not supposed to have access, then an else could be added to reject. I'm open to doing this later when new types are added.

[riemanli]

Dismissed @Marco-Premier from 2 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Marco-Premier and @tristanvuong2021)

---

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

Previously, stevenwarejones (Steven Ware Jones) wrote…

i guess this depends on the desired behavior. If all other principal types are not supposed to have access, then an else could be added to reject. I'm open to doing this later when new types are added.

Sounds good. Thanks Steven!

[riemanli]

Reviewed 1 of 1 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Marco-Premier and @tristanvuong2021)

[riemanli]

@riemanli queued this pull request to merge with Graphite.

[riemanli]

@riemanli merged this pull request with Graphite.