Add a secondary EventGroup pattern with MeasurementConsumer as the parent.

[SanjayVas]

No description provided.

[wfa-reviewable]

This change is 

[stevenwarejones]

Reviewed 1 of 2 files at r1, all commit messages.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @SanjayVas)

---

src/main/proto/wfa/measurement/api/v2alpha/event_groups_service.proto line 138 at r1 (raw file):

        [(google.api.resource_reference).type =
             "halo.wfanet.org/MeasurementConsumer"];
    // Matches against the parent `DataProvider`.

are these comments backwards? eg... if I have a MeasurementConsumer, I filter my results by DataProvider and vice sersa?

---

src/main/proto/wfa/measurement/api/v2alpha/event_groups_service.proto line 139 at r1 (raw file):

             "halo.wfanet.org/MeasurementConsumer"];
    // Matches against the parent `DataProvider`.
    repeated string data_providers = 6

should these be a oneof? Both data_providers and model_providers shouldn't be specified

[SanjayVas]

Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @stevenwarejones)

---

src/main/proto/wfa/measurement/api/v2alpha/event_groups_service.proto line 138 at r1 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

are these comments backwards? eg... if I have a MeasurementConsumer, I filter my results by DataProvider and vice sersa?

Not sure what you mean. The measurement_consumers filter field has a comment stating it matches against the measurement_consumer field in the resource message. The data_providers filter field has a comment stating it matches against the parent DataProvider of the resource.

---

src/main/proto/wfa/measurement/api/v2alpha/event_groups_service.proto line 139 at r1 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

should these be a oneof? Both data_providers and model_providers shouldn't be specified

That's only because of our current auth scheme, where the only principals that can call this method are MeasurementConsumers and DataProviders and that each can only list their own child EventGroups. If we happened to allow some admin principal type that can list across all MCs and EDPs, then that caller could theoretically make use of both filters.

I don't think we want to be that prescriptive in filter messages. You can filter however you want. e.g. if the caller is a DataProvider and is therefore using a DataProvider as their request parent, then including the same DataProvider in your filter doesn't change the results. Including a different DataProvider in your filter means there will be no results. Neither of this is an error, just not particularly useful.

[SanjayVas]

For #144

[stevenwarejones]

Reviewed 1 of 2 files at r3, 8 of 8 files at r4, all commit messages.
Reviewable status: 9 of 10 files reviewed, 1 unresolved discussion (waiting on @SanjayVas)

---

src/main/proto/wfa/measurement/api/v2alpha/event_groups_service.proto line 138 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Not sure what you mean. The measurement_consumers filter field has a comment stating it matches against the measurement_consumer field in the resource message. The data_providers filter field has a comment stating it matches against the parent DataProvider of the resource.

The comment reads to me that it matches against the parent DataProvider in addition to the provide list of data_providers. In actuality, it filters against the parent MeasurementConsumer data_providers is populated.

I think both these comments should be combined together.

The Filter filters first by the parent resource and then by the subset that matches the filter fields.

---

src/main/proto/wfa/measurement/api/v2alpha/event_groups_service.proto line 139 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

That's only because of our current auth scheme, where the only principals that can call this method are MeasurementConsumers and DataProviders and that each can only list their own child EventGroups. If we happened to allow some admin principal type that can list across all MCs and EDPs, then that caller could theoretically make use of both filters.

I don't think we want to be that prescriptive in filter messages. You can filter however you want. e.g. if the caller is a DataProvider and is therefore using a DataProvider as their request parent, then including the same DataProvider in your filter doesn't change the results. Including a different DataProvider in your filter means there will be no results. Neither of this is an error, just not particularly useful.

okay. this makes sense if we have bette commenting. see my other comment.

[SanjayVas]

Reviewable status: 9 of 10 files reviewed, 1 unresolved discussion (waiting on @stevenwarejones)

---

src/main/proto/wfa/measurement/api/v2alpha/event_groups_service.proto line 138 at r1 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

The comment reads to me that it matches against the parent DataProvider in addition to the provide list of data_providers. In actuality, it filters against the parent MeasurementConsumer data_providers is populated.

I think both these comments should be combined together.

The Filter filters first by the parent resource and then by the subset that matches the filter fields.

A filter field in a List method always filters against the resource. Each comment is indicating which part of resource that the subfield is matching. The measurement_consumers filter field is matching against the measurement_consumer field of the resource. The data_providers field is matching against the parent DataProvider of the resource (which is an inherent part of the resource name).

This is just a structured version of what would otherwise be supported by AIP-160. e.g.

filter {
  measurement_consumers: "measurementConsumers/abc123"
  measurement_consumers: "measurementConsumers/xyz456"
}

Translates to measurement_consumer = "measurementConsumers/abc123" OR measurement_consumer = "measurementConsumers/xyz456"

The data_providers field in the Filter message translates slightly differently as the EventGroup resource message doesn't have a data_provider field, and that is instead indicated by the prefix of the resource name.

filter {
  data_providers: "dataProvider/abc123"
}

Translates to name = "dataProviders/abc123/*

[SanjayVas]

Reviewable status: 9 of 10 files reviewed, 1 unresolved discussion (waiting on @stevenwarejones)

---

src/main/proto/wfa/measurement/api/v2alpha/event_groups_service.proto line 138 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

A filter field in a List method always filters against the resource. Each comment is indicating which part of resource that the subfield is matching. The measurement_consumers filter field is matching against the measurement_consumer field of the resource. The data_providers field is matching against the parent DataProvider of the resource (which is an inherent part of the resource name).

This is just a structured version of what would otherwise be supported by AIP-160. e.g.

filter {
  measurement_consumers: "measurementConsumers/abc123"
  measurement_consumers: "measurementConsumers/xyz456"
}

Translates to measurement_consumer = "measurementConsumers/abc123" OR measurement_consumer = "measurementConsumers/xyz456"

The data_providers field in the Filter message translates slightly differently as the EventGroup resource message doesn't have a data_provider field, and that is instead indicated by the prefix of the resource name.

filter {
  data_providers: "dataProvider/abc123"
}

Translates to name = "dataProviders/abc123/*

Clarifications just in case it's unclear:

• The filters will apply to the resource collection indicated by the parent field in the request. If the parent is measurementConsumers/abc123, then it means the collection you're referring to is EventGroups belonging to that MC. Further filtering by the measurement_consumer field is therefore unnecessary.
• The canonical parent of the eventGroups resource collection is DataProvider. This is evidenced by the fact that he the canonical format of the EventGroup resource name is dataProviders/{data_provider}/eventGroups/{event_group}. That is the format that the service will always return regardless of what format is used for the parent of the ListEventGroups request.

[stevenwarejones]

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

---

src/main/proto/wfa/measurement/api/v2alpha/event_groups_service.proto line 138 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Clarifications just in case it's unclear:

• The filters will apply to the resource collection indicated by the parent field in the request. If the parent is measurementConsumers/abc123, then it means the collection you're referring to is EventGroups belonging to that MC. Further filtering by the measurement_consumer field is therefore unnecessary.
• The canonical parent of the eventGroups resource collection is DataProvider. This is evidenced by the fact that he the canonical format of the EventGroup resource name is dataProviders/{data_provider}/eventGroups/{event_group}. That is the format that the service will always return regardless of what format is used for the parent of the ListEventGroups request.

^The filters will apply to the resource collection indicated by the parent field in the request. If the parent is measurementConsumers/abc123, then it means the collection you're referring to is EventGroups belonging to that MC. Further filtering by the measurement_consumer field is therefore unnecessary.

I agree with everything you said. I think its confusing potentially to an adopter that if you are a measurementConsumer then there is no point to filling out the measurement_consumer field. I think the comments should reflect that.

[SanjayVas]

Reviewable status: 9 of 10 files reviewed, 1 unresolved discussion (waiting on @stevenwarejones)

---

src/main/proto/wfa/measurement/api/v2alpha/event_groups_service.proto line 138 at r1 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

^The filters will apply to the resource collection indicated by the parent field in the request. If the parent is measurementConsumers/abc123, then it means the collection you're referring to is EventGroups belonging to that MC. Further filtering by the measurement_consumer field is therefore unnecessary.

I agree with everything you said. I think its confusing potentially to an adopter that if you are a measurementConsumer then there is no point to filling out the measurement_consumer field. I think the comments should reflect that.

It's unnecessary but also doesn't do any harm.

Again, this is just the filtering feature that the AIPs describe but with a more limited structure instead of supporting the entire expression language. I want to avoid having the filter documentation be aware of specific use cases. It shouldn't describe anything more than the resulting predicate.

[stevenwarejones]

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