feat: Add Basic Report #1942
feat: Add Basic Report #1942
Conversation
|
This change is |
kungfucraig
changed the title
There was a problem hiding this comment.
Reviewed 1 of 3 files at r1, 1 of 2 files at r3, all commit messages.
Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @kungfucraig, @mariolamassaavedra, @SanjayVas, and @tristanvuong2021)
src/main/proto/wfa/measurement/reporting/v2alpha/media_type.proto line 29 at r3 (raw file):
// CROSS_MEDIA is used when the artifact encompases more that one // of the other types. enum MediaType {
Are these validated by Alex and Hannah?
There was a problem hiding this comment.
Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @kungfucraig, @mariolamassaavedra, @SanjayVas, and @tristanvuong2021)
src/main/proto/wfa/measurement/reporting/v2alpha/media_type.proto line 29 at r3 (raw file):
Previously, stevenwarejones (Steven Ware Jones) wrote…
Are these validated by Alex and Hannah?
Also - isn't there some talk of sub-categories as well?
There was a problem hiding this comment.
Reviewable status: 2 of 3 files reviewed, 3 unresolved discussions (waiting on @kungfucraig, @mariolamassaavedra, @SanjayVas, and @tristanvuong2021)
src/main/proto/wfa/measurement/reporting/v2alpha/basic_report.proto line 56 at r3 (raw file):
// be reported on. Event Groups may span Data Providers and Media Types. // Subsets of these can be reported on and are determined by PageSpec. string campaign_group = 3;
is this the reporting_set_resource_name?
src/main/proto/wfa/measurement/reporting/v2alpha/basic_report.proto line 235 at r3 (raw file):
// Discuss: There should probably be an option for week over week unique // too? Should check the modeling API proposal to see if that's necessary. // bool unique = 3;
did you mean to comment this out?
There was a problem hiding this comment.
Reviewable status: 2 of 3 files reviewed, 3 unresolved discussions (waiting on @kungfucraig, @mariolamassaavedra, @SanjayVas, and @tristanvuong2021)
src/main/proto/wfa/measurement/reporting/v2alpha/media_type.proto line 29 at r3 (raw file):
Previously, stevenwarejones (Steven Ware Jones) wrote…
Also - isn't there some talk of sub-categories as well?
I'd prefer to not have Other as an option since this list is likely to grow. The definition of other will be reduced over time possibly leading to some confusion.
There was a problem hiding this comment.
@stevenwarejones This definition should support Phase I and Phase II. My plan is to cull this to the set of things necessary for Phase I before merging, but I'd like to get buy-in on Phase II since the definitions for Phase I need to be compatible with Phase II.
Reviewable status: 2 of 3 files reviewed, 3 unresolved discussions (waiting on @mariolamassaavedra, @SanjayVas, @stevenwarejones, and @tristanvuong2021)
src/main/proto/wfa/measurement/reporting/v2alpha/basic_report.proto line 56 at r3 (raw file):
Previously, stevenwarejones (Steven Ware Jones) wrote…
is this the reporting_set_resource_name?
It's the name of a reporting set, yes. However, it's not any old reporting set, it has a particular structure (see comment), which is why the term campaign group is used.
src/main/proto/wfa/measurement/reporting/v2alpha/basic_report.proto line 235 at r3 (raw file):
Previously, stevenwarejones (Steven Ware Jones) wrote…
did you mean to comment this out?
Yes.
src/main/proto/wfa/measurement/reporting/v2alpha/media_type.proto line 29 at r3 (raw file):
Previously, stevenwarejones (Steven Ware Jones) wrote…
I'd prefer to not have Other as an option since this list is likely to grow. The definition of other will be reduced over time possibly leading to some confusion.
No, but let's work under the assumption that this list can grow and that the market config can choose to use a subset of these.
There is talk of sub-categories, but it's not clear that the sub-categories are being used to drive reporting behavior.
The good news is that we don't need this for Phase I.
There was a problem hiding this comment.
Reviewable status: 2 of 3 files reviewed, 3 unresolved discussions (waiting on @mariolamassaavedra, @SanjayVas, @stevenwarejones, and @tristanvuong2021)
src/main/proto/wfa/measurement/reporting/v2alpha/media_type.proto line 29 at r3 (raw file):
Previously, kungfucraig (Craig Wright) wrote…
No, but let's work under the assumption that this list can grow and that the market config can choose to use a subset of these.
There is talk of sub-categories, but it's not clear that the sub-categories are being used to drive reporting behavior.
The good news is that we don't need this for Phase I.
Note that "Other" is an existing media type in the current Origin report. Given that I think we have to have it here.
There was a problem hiding this comment.
Reviewed 1 of 3 files at r1, 1 of 2 files at r3, all commit messages.
Reviewable status: 2 of 3 files reviewed, 6 unresolved discussions (waiting on @kungfucraig, @mariolamassaavedra, @SanjayVas, and @stevenwarejones)
src/main/proto/wfa/measurement/reporting/v2alpha/basic_report.proto line 84 at r3 (raw file):
ReportingInterval reporting_interval = 4; // Specification of for custom viewability and completion filter.
nit: Is there a word missing here or is the of extra?
src/main/proto/wfa/measurement/reporting/v2alpha/basic_report.proto line 149 at r3 (raw file):
message Filter { message Term { // The name of event template (aka dimension) to filter on
Is this supposed to be event template field
src/main/proto/wfa/measurement/reporting/v2alpha/basic_report.proto line 363 at r3 (raw file):
// The qualification filter used when computing the result. oneof qualification_filter { string name = 6;
What is the name here
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: 2 of 3 files reviewed, 7 unresolved discussions (waiting on @kungfucraig, @mariolamassaavedra, and @stevenwarejones)
a discussion (no related file):
The API lint GitHub workflow does not currently cover this API. I'm willing to bet there are other API lint errors elsewhere in the API definition. For now, just make sure that you run the linter yourself using tools/api-lint to make sure that this PR does not introduce any new lint warnings. I've included the initial findings below. The main issue is that every symbol must be documented.
- file_path: wfa/measurement/reporting/v2alpha/basic_report.proto
problems:
- message: '"value" should have only leading (not trailing) public comments.'
location:
start_position:
line_number: 152
column_number: 7
end_position:
line_number: 152
column_number: 23
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::only-leading-comments
rule_doc_uri: https://linter.aip.dev/192/only-leading-comments
- message: '"cumulative" should have only leading (not trailing) public comments.'
location:
start_position:
line_number: 226
column_number: 7
end_position:
line_number: 226
column_number: 26
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::only-leading-comments
rule_doc_uri: https://linter.aip.dev/192/only-leading-comments
- message: '"DATA_PROVIDER" should have only leading (not trailing) public comments.'
location:
start_position:
line_number: 166
column_number: 5
end_position:
line_number: 166
column_number: 22
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::only-leading-comments
rule_doc_uri: https://linter.aip.dev/192/only-leading-comments
- message: Resources should declare plural.
location:
start_position:
line_number: 39
column_number: 3
end_position:
line_number: 42
column_number: 4
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0123::resource-plural
rule_doc_uri: https://linter.aip.dev/123/resource-plural
- message: The first enum value should be "REPORTING_UNIT_TYPE_UNSPECIFIED"
suggestion: REPORTING_UNIT_TYPE_UNSPECIFIED
location:
start_position:
line_number: 164
column_number: 5
end_position:
line_number: 164
column_number: 30
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0126::unspecified
rule_doc_uri: https://linter.aip.dev/126/unspecified
- message: Fields should not use the `_name` suffix.
suggestion: dimension
location:
start_position:
line_number: 140
column_number: 21
end_position:
line_number: 140
column_number: 34
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0122::name-suffix
rule_doc_uri: https://linter.aip.dev/122/name-suffix
- message: Fields should not use the `_name` suffix.
suggestion: dimension
location:
start_position:
line_number: 150
column_number: 14
end_position:
line_number: 150
column_number: 27
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0122::name-suffix
rule_doc_uri: https://linter.aip.dev/122/name-suffix
- message: Fields should not use the `_name` suffix.
suggestion: dimension
location:
start_position:
line_number: 369
column_number: 16
end_position:
line_number: 369
column_number: 29
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0122::name-suffix
rule_doc_uri: https://linter.aip.dev/122/name-suffix
- message: 'Resources should declare singular: "basicReport"'
location:
start_position:
line_number: 39
column_number: 3
end_position:
line_number: 42
column_number: 4
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0123::resource-singular
rule_doc_uri: https://linter.aip.dev/123/resource-singular
- message: resource name field must have field_behavior IDENTIFIER
location:
start_position:
line_number: 45
column_number: 3
end_position:
line_number: 45
column_number: 18
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0203::resource-name-identifier
rule_doc_uri: https://linter.aip.dev/203/resource-name-identifier
- message: Missing comment over "reporting_interval".
location:
start_position:
line_number: 82
column_number: 21
end_position:
line_number: 82
column_number: 38
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::has-comments
rule_doc_uri: https://linter.aip.dev/192/has-comments
- message: Missing comment over "viewability".
location:
start_position:
line_number: 103
column_number: 13
end_position:
line_number: 103
column_number: 23
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::has-comments
rule_doc_uri: https://linter.aip.dev/192/has-comments
- message: Missing comment over "completion".
location:
start_position:
line_number: 104
column_number: 13
end_position:
line_number: 104
column_number: 22
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::has-comments
rule_doc_uri: https://linter.aip.dev/192/has-comments
- message: Missing comment over "fraction".
location:
start_position:
line_number: 99
column_number: 15
end_position:
line_number: 99
column_number: 22
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::has-comments
rule_doc_uri: https://linter.aip.dev/192/has-comments
- message: Missing comment over "quantile".
location:
start_position:
line_number: 100
column_number: 18
end_position:
line_number: 100
column_number: 25
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::has-comments
rule_doc_uri: https://linter.aip.dev/192/has-comments
- message: Missing comment over "Term".
location:
start_position:
line_number: 148
column_number: 13
end_position:
line_number: 148
column_number: 16
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::has-comments
rule_doc_uri: https://linter.aip.dev/192/has-comments
- message: Missing comment over "longitudinal_options".
location:
start_position:
line_number: 237
column_number: 25
end_position:
line_number: 237
column_number: 44
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::has-comments
rule_doc_uri: https://linter.aip.dev/192/has-comments
- message: Missing comment over "day_of_week".
location:
start_position:
line_number: 209
column_number: 31
end_position:
line_number: 209
column_number: 41
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::has-comments
rule_doc_uri: https://linter.aip.dev/192/has-comments
- message: Missing comment over "metric_spec".
location:
start_position:
line_number: 322
column_number: 18
end_position:
line_number: 322
column_number: 28
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::has-comments
rule_doc_uri: https://linter.aip.dev/192/has-comments
- message: Missing comment over "anchored_reach_specs".
location:
start_position:
line_number: 323
column_number: 26
end_position:
line_number: 323
column_number: 45
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::has-comments
rule_doc_uri: https://linter.aip.dev/192/has-comments
- message: Missing comment over "metric_type".
location:
start_position:
line_number: 312
column_number: 18
end_position:
line_number: 312
column_number: 28
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::has-comments
rule_doc_uri: https://linter.aip.dev/192/has-comments
- message: Missing comment over "specs".
location:
start_position:
line_number: 317
column_number: 34
end_position:
line_number: 317
column_number: 38
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::has-comments
rule_doc_uri: https://linter.aip.dev/192/has-comments
- message: Missing comment over "name".
location:
start_position:
line_number: 363
column_number: 16
end_position:
line_number: 363
column_number: 19
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::has-comments
rule_doc_uri: https://linter.aip.dev/192/has-comments
- message: Missing comment over "custom".
location:
start_position:
line_number: 364
column_number: 45
end_position:
line_number: 364
column_number: 50
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::has-comments
rule_doc_uri: https://linter.aip.dev/192/has-comments
- message: Missing comment over "name".
location:
start_position:
line_number: 344
column_number: 16
end_position:
line_number: 344
column_number: 19
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::has-comments
rule_doc_uri: https://linter.aip.dev/192/has-comments
- message: Missing comment over "display_name".
location:
start_position:
line_number: 345
column_number: 16
end_position:
line_number: 345
column_number: 27
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::has-comments
rule_doc_uri: https://linter.aip.dev/192/has-comments
- message: Missing comment over "reporting_unit_type".
location:
start_position:
line_number: 350
column_number: 27
end_position:
line_number: 350
column_number: 45
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::has-comments
rule_doc_uri: https://linter.aip.dev/192/has-comments
- message: Missing comment over "reporting_unit_summary".
location:
start_position:
line_number: 351
column_number: 39
end_position:
line_number: 351
column_number: 60
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::has-comments
rule_doc_uri: https://linter.aip.dev/192/has-comments
- message: Missing comment over "dimension_name".
location:
start_position:
line_number: 369
column_number: 16
end_position:
line_number: 369
column_number: 29
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::has-comments
rule_doc_uri: https://linter.aip.dev/192/has-comments
- message: Missing comment over "value".
location:
start_position:
line_number: 370
column_number: 16
end_position:
line_number: 370
column_number: 20
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::has-comments
rule_doc_uri: https://linter.aip.dev/192/has-comments
- message: Missing comment over "metric_result".
location:
start_position:
line_number: 411
column_number: 22
end_position:
line_number: 411
column_number: 34
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::has-comments
rule_doc_uri: https://linter.aip.dev/192/has-comments
- message: Missing comment over "anchored_reach_results".
location:
start_position:
line_number: 412
column_number: 30
end_position:
line_number: 412
column_number: 51
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::has-comments
rule_doc_uri: https://linter.aip.dev/192/has-comments
- message: Missing comment over "reach".
location:
start_position:
line_number: 385
column_number: 15
end_position:
line_number: 385
column_number: 19
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::has-comments
rule_doc_uri: https://linter.aip.dev/192/has-comments
- message: Missing comment over "percent_reach".
location:
start_position:
line_number: 386
column_number: 15
end_position:
line_number: 386
column_number: 27
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::has-comments
rule_doc_uri: https://linter.aip.dev/192/has-comments
- message: Missing comment over "percent_k_plus_reach".
location:
start_position:
line_number: 389
column_number: 24
end_position:
line_number: 389
column_number: 43
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::has-comments
rule_doc_uri: https://linter.aip.dev/192/has-comments
- message: Missing comment over "unique_reach".
location:
start_position:
line_number: 390
column_number: 15
end_position:
line_number: 390
column_number: 26
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::has-comments
rule_doc_uri: https://linter.aip.dev/192/has-comments
- message: Missing comment over "average_frequency".
location:
start_position:
line_number: 391
column_number: 15
end_position:
line_number: 391
column_number: 31
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::has-comments
rule_doc_uri: https://linter.aip.dev/192/has-comments
- message: Missing comment over "impressions".
location:
start_position:
line_number: 392
column_number: 15
end_position:
line_number: 392
column_number: 25
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::has-comments
rule_doc_uri: https://linter.aip.dev/192/has-comments
- message: Missing comment over "grps".
location:
start_position:
line_number: 393
column_number: 15
end_position:
line_number: 393
column_number: 18
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::has-comments
rule_doc_uri: https://linter.aip.dev/192/has-comments
- message: Missing comment over "metadata".
location:
start_position:
line_number: 419
column_number: 22
end_position:
line_number: 419
column_number: 29
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::has-comments
rule_doc_uri: https://linter.aip.dev/192/has-comments
- message: Missing comment over "value".
location:
start_position:
line_number: 420
column_number: 19
end_position:
line_number: 420
column_number: 23
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::has-comments
rule_doc_uri: https://linter.aip.dev/192/has-comments
- message: Missing comment over "ReportingUnitType".
location:
start_position:
line_number: 162
column_number: 8
end_position:
line_number: 162
column_number: 24
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::has-comments
rule_doc_uri: https://linter.aip.dev/192/has-comments
- message: Missing comment over "QUANTILE_UNSPECIFIED".
location:
start_position:
line_number: 88
column_number: 7
end_position:
line_number: 88
column_number: 26
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::has-comments
rule_doc_uri: https://linter.aip.dev/192/has-comments
- message: Missing comment over "ZERO".
location:
start_position:
line_number: 89
column_number: 7
end_position:
line_number: 89
column_number: 10
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::has-comments
rule_doc_uri: https://linter.aip.dev/192/has-comments
- message: Missing comment over "TWENTY_FIVE".
location:
start_position:
line_number: 90
column_number: 7
end_position:
line_number: 90
column_number: 17
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::has-comments
rule_doc_uri: https://linter.aip.dev/192/has-comments
- message: Missing comment over "FIFTY".
location:
start_position:
line_number: 91
column_number: 7
end_position:
line_number: 91
column_number: 11
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::has-comments
rule_doc_uri: https://linter.aip.dev/192/has-comments
- message: Missing comment over "SEVENTY_FIVE".
location:
start_position:
line_number: 92
column_number: 7
end_position:
line_number: 92
column_number: 18
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::has-comments
rule_doc_uri: https://linter.aip.dev/192/has-comments
- message: Missing comment over "ONE_HUNDRED".
location:
start_position:
line_number: 93
column_number: 7
end_position:
line_number: 93
column_number: 17
path: wfa/measurement/reporting/v2alpha/basic_report.proto
rule_id: core::0192::has-comments
rule_doc_uri: https://linter.aip.dev/192/has-comments
There was a problem hiding this comment.
Reviewable status: 2 of 3 files reviewed, 7 unresolved discussions (waiting on @mariolamassaavedra, @SanjayVas, and @stevenwarejones)
a discussion (no related file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
The API lint GitHub workflow does not currently cover this API. I'm willing to bet there are other API lint errors elsewhere in the API definition. For now, just make sure that you run the linter yourself using
tools/api-lintto make sure that this PR does not introduce any new lint warnings. I've included the initial findings below. The main issue is that every symbol must be documented.- file_path: wfa/measurement/reporting/v2alpha/basic_report.proto problems: - message: '"value" should have only leading (not trailing) public comments.' location: start_position: line_number: 152 column_number: 7 end_position: line_number: 152 column_number: 23 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::only-leading-comments rule_doc_uri: https://linter.aip.dev/192/only-leading-comments - message: '"cumulative" should have only leading (not trailing) public comments.' location: start_position: line_number: 226 column_number: 7 end_position: line_number: 226 column_number: 26 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::only-leading-comments rule_doc_uri: https://linter.aip.dev/192/only-leading-comments - message: '"DATA_PROVIDER" should have only leading (not trailing) public comments.' location: start_position: line_number: 166 column_number: 5 end_position: line_number: 166 column_number: 22 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::only-leading-comments rule_doc_uri: https://linter.aip.dev/192/only-leading-comments - message: Resources should declare plural. location: start_position: line_number: 39 column_number: 3 end_position: line_number: 42 column_number: 4 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0123::resource-plural rule_doc_uri: https://linter.aip.dev/123/resource-plural - message: The first enum value should be "REPORTING_UNIT_TYPE_UNSPECIFIED" suggestion: REPORTING_UNIT_TYPE_UNSPECIFIED location: start_position: line_number: 164 column_number: 5 end_position: line_number: 164 column_number: 30 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0126::unspecified rule_doc_uri: https://linter.aip.dev/126/unspecified - message: Fields should not use the `_name` suffix. suggestion: dimension location: start_position: line_number: 140 column_number: 21 end_position: line_number: 140 column_number: 34 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0122::name-suffix rule_doc_uri: https://linter.aip.dev/122/name-suffix - message: Fields should not use the `_name` suffix. suggestion: dimension location: start_position: line_number: 150 column_number: 14 end_position: line_number: 150 column_number: 27 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0122::name-suffix rule_doc_uri: https://linter.aip.dev/122/name-suffix - message: Fields should not use the `_name` suffix. suggestion: dimension location: start_position: line_number: 369 column_number: 16 end_position: line_number: 369 column_number: 29 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0122::name-suffix rule_doc_uri: https://linter.aip.dev/122/name-suffix - message: 'Resources should declare singular: "basicReport"' location: start_position: line_number: 39 column_number: 3 end_position: line_number: 42 column_number: 4 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0123::resource-singular rule_doc_uri: https://linter.aip.dev/123/resource-singular - message: resource name field must have field_behavior IDENTIFIER location: start_position: line_number: 45 column_number: 3 end_position: line_number: 45 column_number: 18 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0203::resource-name-identifier rule_doc_uri: https://linter.aip.dev/203/resource-name-identifier - message: Missing comment over "reporting_interval". location: start_position: line_number: 82 column_number: 21 end_position: line_number: 82 column_number: 38 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::has-comments rule_doc_uri: https://linter.aip.dev/192/has-comments - message: Missing comment over "viewability". location: start_position: line_number: 103 column_number: 13 end_position: line_number: 103 column_number: 23 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::has-comments rule_doc_uri: https://linter.aip.dev/192/has-comments - message: Missing comment over "completion". location: start_position: line_number: 104 column_number: 13 end_position: line_number: 104 column_number: 22 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::has-comments rule_doc_uri: https://linter.aip.dev/192/has-comments - message: Missing comment over "fraction". location: start_position: line_number: 99 column_number: 15 end_position: line_number: 99 column_number: 22 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::has-comments rule_doc_uri: https://linter.aip.dev/192/has-comments - message: Missing comment over "quantile". location: start_position: line_number: 100 column_number: 18 end_position: line_number: 100 column_number: 25 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::has-comments rule_doc_uri: https://linter.aip.dev/192/has-comments - message: Missing comment over "Term". location: start_position: line_number: 148 column_number: 13 end_position: line_number: 148 column_number: 16 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::has-comments rule_doc_uri: https://linter.aip.dev/192/has-comments - message: Missing comment over "longitudinal_options". location: start_position: line_number: 237 column_number: 25 end_position: line_number: 237 column_number: 44 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::has-comments rule_doc_uri: https://linter.aip.dev/192/has-comments - message: Missing comment over "day_of_week". location: start_position: line_number: 209 column_number: 31 end_position: line_number: 209 column_number: 41 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::has-comments rule_doc_uri: https://linter.aip.dev/192/has-comments - message: Missing comment over "metric_spec". location: start_position: line_number: 322 column_number: 18 end_position: line_number: 322 column_number: 28 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::has-comments rule_doc_uri: https://linter.aip.dev/192/has-comments - message: Missing comment over "anchored_reach_specs". location: start_position: line_number: 323 column_number: 26 end_position: line_number: 323 column_number: 45 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::has-comments rule_doc_uri: https://linter.aip.dev/192/has-comments - message: Missing comment over "metric_type". location: start_position: line_number: 312 column_number: 18 end_position: line_number: 312 column_number: 28 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::has-comments rule_doc_uri: https://linter.aip.dev/192/has-comments - message: Missing comment over "specs". location: start_position: line_number: 317 column_number: 34 end_position: line_number: 317 column_number: 38 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::has-comments rule_doc_uri: https://linter.aip.dev/192/has-comments - message: Missing comment over "name". location: start_position: line_number: 363 column_number: 16 end_position: line_number: 363 column_number: 19 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::has-comments rule_doc_uri: https://linter.aip.dev/192/has-comments - message: Missing comment over "custom". location: start_position: line_number: 364 column_number: 45 end_position: line_number: 364 column_number: 50 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::has-comments rule_doc_uri: https://linter.aip.dev/192/has-comments - message: Missing comment over "name". location: start_position: line_number: 344 column_number: 16 end_position: line_number: 344 column_number: 19 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::has-comments rule_doc_uri: https://linter.aip.dev/192/has-comments - message: Missing comment over "display_name". location: start_position: line_number: 345 column_number: 16 end_position: line_number: 345 column_number: 27 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::has-comments rule_doc_uri: https://linter.aip.dev/192/has-comments - message: Missing comment over "reporting_unit_type". location: start_position: line_number: 350 column_number: 27 end_position: line_number: 350 column_number: 45 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::has-comments rule_doc_uri: https://linter.aip.dev/192/has-comments - message: Missing comment over "reporting_unit_summary". location: start_position: line_number: 351 column_number: 39 end_position: line_number: 351 column_number: 60 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::has-comments rule_doc_uri: https://linter.aip.dev/192/has-comments - message: Missing comment over "dimension_name". location: start_position: line_number: 369 column_number: 16 end_position: line_number: 369 column_number: 29 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::has-comments rule_doc_uri: https://linter.aip.dev/192/has-comments - message: Missing comment over "value". location: start_position: line_number: 370 column_number: 16 end_position: line_number: 370 column_number: 20 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::has-comments rule_doc_uri: https://linter.aip.dev/192/has-comments - message: Missing comment over "metric_result". location: start_position: line_number: 411 column_number: 22 end_position: line_number: 411 column_number: 34 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::has-comments rule_doc_uri: https://linter.aip.dev/192/has-comments - message: Missing comment over "anchored_reach_results". location: start_position: line_number: 412 column_number: 30 end_position: line_number: 412 column_number: 51 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::has-comments rule_doc_uri: https://linter.aip.dev/192/has-comments - message: Missing comment over "reach". location: start_position: line_number: 385 column_number: 15 end_position: line_number: 385 column_number: 19 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::has-comments rule_doc_uri: https://linter.aip.dev/192/has-comments - message: Missing comment over "percent_reach". location: start_position: line_number: 386 column_number: 15 end_position: line_number: 386 column_number: 27 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::has-comments rule_doc_uri: https://linter.aip.dev/192/has-comments - message: Missing comment over "percent_k_plus_reach". location: start_position: line_number: 389 column_number: 24 end_position: line_number: 389 column_number: 43 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::has-comments rule_doc_uri: https://linter.aip.dev/192/has-comments - message: Missing comment over "unique_reach". location: start_position: line_number: 390 column_number: 15 end_position: line_number: 390 column_number: 26 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::has-comments rule_doc_uri: https://linter.aip.dev/192/has-comments - message: Missing comment over "average_frequency". location: start_position: line_number: 391 column_number: 15 end_position: line_number: 391 column_number: 31 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::has-comments rule_doc_uri: https://linter.aip.dev/192/has-comments - message: Missing comment over "impressions". location: start_position: line_number: 392 column_number: 15 end_position: line_number: 392 column_number: 25 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::has-comments rule_doc_uri: https://linter.aip.dev/192/has-comments - message: Missing comment over "grps". location: start_position: line_number: 393 column_number: 15 end_position: line_number: 393 column_number: 18 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::has-comments rule_doc_uri: https://linter.aip.dev/192/has-comments - message: Missing comment over "metadata". location: start_position: line_number: 419 column_number: 22 end_position: line_number: 419 column_number: 29 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::has-comments rule_doc_uri: https://linter.aip.dev/192/has-comments - message: Missing comment over "value". location: start_position: line_number: 420 column_number: 19 end_position: line_number: 420 column_number: 23 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::has-comments rule_doc_uri: https://linter.aip.dev/192/has-comments - message: Missing comment over "ReportingUnitType". location: start_position: line_number: 162 column_number: 8 end_position: line_number: 162 column_number: 24 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::has-comments rule_doc_uri: https://linter.aip.dev/192/has-comments - message: Missing comment over "QUANTILE_UNSPECIFIED". location: start_position: line_number: 88 column_number: 7 end_position: line_number: 88 column_number: 26 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::has-comments rule_doc_uri: https://linter.aip.dev/192/has-comments - message: Missing comment over "ZERO". location: start_position: line_number: 89 column_number: 7 end_position: line_number: 89 column_number: 10 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::has-comments rule_doc_uri: https://linter.aip.dev/192/has-comments - message: Missing comment over "TWENTY_FIVE". location: start_position: line_number: 90 column_number: 7 end_position: line_number: 90 column_number: 17 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::has-comments rule_doc_uri: https://linter.aip.dev/192/has-comments - message: Missing comment over "FIFTY". location: start_position: line_number: 91 column_number: 7 end_position: line_number: 91 column_number: 11 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::has-comments rule_doc_uri: https://linter.aip.dev/192/has-comments - message: Missing comment over "SEVENTY_FIVE". location: start_position: line_number: 92 column_number: 7 end_position: line_number: 92 column_number: 18 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::has-comments rule_doc_uri: https://linter.aip.dev/192/has-comments - message: Missing comment over "ONE_HUNDRED". location: start_position: line_number: 93 column_number: 7 end_position: line_number: 93 column_number: 17 path: wfa/measurement/reporting/v2alpha/basic_report.proto rule_id: core::0192::has-comments rule_doc_uri: https://linter.aip.dev/192/has-comments
I will lint it and add annotations, etc once we agree on the substance.
There was a problem hiding this comment.
Reviewed 1 of 3 files at r1.
Reviewable status: 2 of 3 files reviewed, 16 unresolved discussions (waiting on @kungfucraig, @mariolamassaavedra, and @stevenwarejones)
src/main/proto/wfa/measurement/reporting/v2alpha/basic_report.proto line 56 at r3 (raw file):
Previously, kungfucraig (Craig Wright) wrote…
It's the name of a reporting set, yes. However, it's not any old reporting set, it has a particular structure (see comment), which is why the term campaign group is used.
A resource_reference annotation would make this clearer.
src/main/proto/wfa/measurement/reporting/v2alpha/basic_report.proto line 363 at r3 (raw file):
This cannot be called name since it is not the resource name of the message in which it is defined.
From AIP-122:
Fields must not be called
nameexcept for this purpose. For other use cases, either use a different term or prepend an adjective (for example:display_name).
src/main/proto/wfa/measurement/reporting/v2alpha/media_type.proto line 33 at r3 (raw file):
MEDIA_TYPE_UNSPECIFIED = 0; // The artifact is a mix of MediaTypes CROSS_MEDIA = 1;
Do we want CROSS_MEDIA as its own type, or instead represent this as a repeated of MediaType? For example, if we're using this enum on EventGroup just marking it as CROSS_MEDIA isn't helpful. Knowing it has more than one media type lets us know that it's cross-media.
src/main/proto/wfa/measurement/reporting/v2alpha/basic_report.proto line 88 at r3 (raw file):
// Specification for a quantile enum Quantile { QUANTILE_UNSPECIFIED = 0;
Is there a meaningful difference between unspecified and zero? Is it something like whether video playback even started?
src/main/proto/wfa/measurement/reporting/v2alpha/basic_report.proto line 122 at r3 (raw file):
// The valid values and their configurations are specified as // part of the local market Measurement Policy. repeated string filter_names = 2;
Rather than having these be special strings, I think we can have ImpressionFilter be a separate resource type and have these be resource references. The resource itself need not have Create methods and can just come from a config file.
This allows for discoverability of the supported filters for a Reporting system instance (market).
src/main/proto/wfa/measurement/reporting/v2alpha/basic_report.proto line 139 at r3 (raw file):
// // Only those event template fields that are tagged as groupable // and in the common template are supported.
- How is the common template identified? Is there going to be a new field in the template annotation?
- Need to be more specific on the format here. For example, is this a field name within the common template, or is this a field path on the event message? (
age_groupvs.common.age_group)
Maybe dimension_field or dimension_field_name to be clearer?
Code quote:
// Only those event template fields that are tagged as groupable
// and in the common template are supported.src/main/proto/wfa/measurement/reporting/v2alpha/basic_report.proto line 152 at r3 (raw file):
string dimension_name = 1; // The value of the event template to filter string value = 2;
I assume field types other than string should be supported. This should use a structure like FieldValue in simulator_synthetic_data_spec.proto.
src/main/proto/wfa/measurement/reporting/v2alpha/basic_report.proto line 165 at r3 (raw file):
// Default value. Unused. REPORTING_TYPE_UNSPECIFIED = 0; // The artifact used by the reporting unit is a Data Provider name
nit: either explicitly say "resource name" or just leave the "name" part out as all resource references are resource names.
I think it would also be good to include the formal resource type name in this documentation. This can be found in the resource annotation on the resource type. The one for DataProvider is halo.wfanet.org/DataProvider.
Code quote:
namesrc/main/proto/wfa/measurement/reporting/v2alpha/basic_report.proto line 179 at r3 (raw file):
ReportingUnitType reporting_unit_type = 1; // A list of resource names that correspond to the reporting unit type. repeated string reporting_units = 2;
Add a resource reference annotation here. You can use "*" as the type if multiple resource types are supported. See the protected_resource field on wfa.measurement.access.v1alpha.Policy for an example.
src/main/proto/wfa/measurement/reporting/v2alpha/basic_report.proto line 384 at r3 (raw file):
// The result when the page selector is metric_type message MetricResult {
Can this reuse the MetricType message?
There was a problem hiding this comment.
Reviewable status: 2 of 3 files reviewed, 16 unresolved discussions (waiting on @kungfucraig, @mariolamassaavedra, and @stevenwarejones)
a discussion (no related file):
Previously, kungfucraig (Craig Wright) wrote…
I will lint it and add annotations, etc once we agree on the substance.
Some annotations are an important part of said substance. Namely, resource references and field behaviors.
There was a problem hiding this comment.
Will add services either in a follow-up or in this PR once the resource definitions are agreed to.
Reviewable status: 0 of 7 files reviewed, 13 unresolved discussions (waiting on @mariolamassaavedra, @SanjayVas, @stevenwarejones, and @tristanvuong2021)
a discussion (no related file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
Some annotations are an important part of said substance. Namely, resource references and field behaviors.
I added annotations. Will lint on a future revision and before merge.
src/main/proto/wfa/measurement/reporting/v2alpha/basic_report.proto line 56 at r3 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
A resource_reference annotation would make this clearer.
Done.
src/main/proto/wfa/measurement/reporting/v2alpha/basic_report.proto line 88 at r3 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
Is there a meaningful difference between unspecified and zero? Is it something like whether video playback even started?
It's just a default that will cause an error to ensure the user sets a value explicitly. Defaulting to zero would make the default custom filter look like "all measured impressions," which is not a great behavior.
src/main/proto/wfa/measurement/reporting/v2alpha/basic_report.proto line 122 at r3 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
Rather than having these be special strings, I think we can have ImpressionFilter be a separate resource type and have these be resource references. The resource itself need not have Create methods and can just come from a config file.
This allows for discoverability of the supported filters for a Reporting system instance (market).
Good idea. Added resource.
src/main/proto/wfa/measurement/reporting/v2alpha/basic_report.proto line 139 at r3 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
- How is the common template identified? Is there going to be a new field in the template annotation?
- Need to be more specific on the format here. For example, is this a field name within the common template, or is this a field path on the event message? (
age_groupvs.common.age_group)Maybe
dimension_fieldordimension_field_nameto be clearer?
I will explore the first question in a forthcoming design doc.
For the second point, I update the comments.
src/main/proto/wfa/measurement/reporting/v2alpha/basic_report.proto line 149 at r3 (raw file):
Previously, tristanvuong2021 (Tristan Vuong) wrote…
Is this supposed to be event template field
Yes. Updated.
src/main/proto/wfa/measurement/reporting/v2alpha/basic_report.proto line 152 at r3 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
I assume field types other than string should be supported. This should use a structure like
FieldValueinsimulator_synthetic_data_spec.proto.
Done.
src/main/proto/wfa/measurement/reporting/v2alpha/basic_report.proto line 179 at r3 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
Add a resource reference annotation here. You can use "*" as the type if multiple resource types are supported. See the
protected_resourcefield onwfa.measurement.access.v1alpha.Policyfor an example.
Done.
src/main/proto/wfa/measurement/reporting/v2alpha/basic_report.proto line 363 at r3 (raw file):
Previously, tristanvuong2021 (Tristan Vuong) wrote…
What is the name here
See page.proto
src/main/proto/wfa/measurement/reporting/v2alpha/basic_report.proto line 363 at r3 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
This cannot be called
namesince it is not the resource name of the message in which it is defined.From AIP-122:
Fields must not be called
nameexcept for this purpose. For other use cases, either use a different term or prepend an adjective (for example:display_name).
Done.
src/main/proto/wfa/measurement/reporting/v2alpha/basic_report.proto line 384 at r3 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
Can this reuse the MetricType message?
No, because the MetricType mostly has booleans to indicate something be computed and this has the actual values.
src/main/proto/wfa/measurement/reporting/v2alpha/media_type.proto line 33 at r3 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
Do we want CROSS_MEDIA as its own type, or instead represent this as a repeated of
MediaType? For example, if we're using this enum on EventGroup just marking it asCROSS_MEDIAisn't helpful. Knowing it has more than one media type lets us know that it's cross-media.
So... I think we actually end up with an enum on the Event Group side that doesn't have CROSS_MEDIA.
There was a problem hiding this comment.
Reviewable status: 0 of 7 files reviewed, 13 unresolved discussions (waiting on @mariolamassaavedra, @SanjayVas, @stevenwarejones, and @tristanvuong2021)
a discussion (no related file):
Previously, kungfucraig (Craig Wright) wrote…
I added annotations. Will lint on a future revision and before merge.
Lint is complete.
There was a problem hiding this comment.
Reviewed 1 of 7 files at r4, 5 of 5 files at r6, all commit messages.
Reviewable status: 6 of 7 files reviewed, 13 unresolved discussions (waiting on @kungfucraig, @mariolamassaavedra, @stevenwarejones, and @tristanvuong2021)
src/main/proto/wfa/measurement/reporting/v2alpha/page.proto line 82 at r6 (raw file):
nit: Use similar language as that on SyntheticPopulationSpec.population_fields.
Field paths use
.as the traveral operator. For example, if there is a field in thecommontemplate namedage_group, then the field path would becommon.age_group.
Code quote:
// For example, if there is an event tempalte field in common
// named "age_group" then this value should be common.age_groupsrc/main/proto/wfa/measurement/reporting/v2alpha/media_type.proto line 33 at r3 (raw file):
Previously, kungfucraig (Craig Wright) wrote…
So... I think we actually end up with an enum on the Event Group side that doesn't have CROSS_MEDIA.
Yes, the fact that the CMMS API will have its own enum is a given. My point is that I don't think I like having a value in an enum that essentially means "some unspecified combination of the other enum values". Meaning I think the two enums should have the same set of values, and that CROSS_MEDIA would never be a valid valid for an enum that's referring to a singular media type.
src/main/proto/wfa/measurement/reporting/v2alpha/reporting_common.proto line 27 at r6 (raw file):
// Specifies the units to report on. message ReportingUnit {
nit: use separate files if there's not a clearer concept for how these top-level definitions are related
src/main/proto/wfa/measurement/reporting/v2alpha/reporting_common.proto line 152 at r6 (raw file):
// // TODO(@kungfucraig) Introduce REPORTING_SET for Phase III enum ReportingUnitType {
I thought we were dropping this enum, and instead having the components field have a resource reference of type halo.wfanet.org/DataProvider for BasicReport and type reporting.halo-cmm.org/ReportingSet for the AdvancedReport query.
src/main/proto/wfa/measurement/reporting/v2alpha/basic_report.proto line 42 at r6 (raw file):
// Resource name. string name = 1 [ (google.api.field_behavior) = IMMUTABLE,
The name field should only have the IDENTIFIER behavior. This inherently implies immutability after creation. The subtlety here is that the name field isn't specified in Create requests.
src/main/proto/wfa/measurement/reporting/v2alpha/basic_report.proto line 93 at r6 (raw file):
// The set of filters for this report. // // An entry in this map must exist for every ReportingMediaType used in
I don't think this is a map
Suggestion:
setsrc/main/proto/wfa/measurement/reporting/v2alpha/page.proto line 31 at r6 (raw file):
// Defines the Frequency, Groupings, and Filters for calculating // a set of Metrics message CalculationSpec {
nit: Move this to its own file if it's used in more than one message. Otherwise nest it inside of the sole message type its used inside of.
src/main/proto/wfa/measurement/reporting/v2alpha/page.proto line 112 at r6 (raw file):
message FieldValue { oneof value { // Used when the event template field is a string or enum
Use a different field for enums to be explicit, even if you want to use the string name as the value instead of the number. e.g.
// Name of the value for a field with an enum type.
string enum_value = 2 [(google.api.field_behavior) = IMMUTABLE];Code quote:
or enum
There was a problem hiding this comment.
Reviewable status: 2 of 19 files reviewed, 11 unresolved discussions (waiting on @mariolamassaavedra, @SanjayVas, @stevenwarejones, and @tristanvuong2021)
src/main/proto/wfa/measurement/reporting/v2alpha/basic_report.proto line 93 at r6 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
I don't think this is a map
Done.
src/main/proto/wfa/measurement/reporting/v2alpha/page.proto line 31 at r6 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
nit: Move this to its own file if it's used in more than one message. Otherwise nest it inside of the sole message type its used inside of.
This is used by two message in this file. The items in this file are very closely related.
src/main/proto/wfa/measurement/reporting/v2alpha/page.proto line 82 at r6 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
nit: Use similar language as that on
SyntheticPopulationSpec.population_fields.Field paths use
.as the traveral operator. For example, if there is a field in thecommontemplate namedage_group, then the field path would becommon.age_group.
Leaving this unresolved and will update later.
src/main/proto/wfa/measurement/reporting/v2alpha/page.proto line 112 at r6 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
Use a different field for enums to be explicit, even if you want to use the string name as the value instead of the number. e.g.
// Name of the value for a field with an enum type. string enum_value = 2 [(google.api.field_behavior) = IMMUTABLE];
Done.
src/main/proto/wfa/measurement/reporting/v2alpha/media_type.proto line 33 at r3 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
Yes, the fact that the CMMS API will have its own enum is a given. My point is that I don't think I like having a value in an enum that essentially means "some unspecified combination of the other enum values". Meaning I think the two enums should have the same set of values, and that
CROSS_MEDIAwould never be a valid valid for an enum that's referring to a singular media type.
Updated an added FilterType. I was trying to use the same enum for both, but splitting them works out pretty well.
src/main/proto/wfa/measurement/reporting/v2alpha/reporting_common.proto line 27 at r6 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
nit: use separate files if there's not a clearer concept for how these top-level definitions are related
Split this file up.
src/main/proto/wfa/measurement/reporting/v2alpha/reporting_common.proto line 152 at r6 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
I thought we were dropping this enum, and instead having the
componentsfield have a resource reference of typehalo.wfanet.org/DataProviderforBasicReportand typereporting.halo-cmm.org/ReportingSetfor theAdvancedReportquery.
Agreed offline to drop it. And it's gone...
There was a problem hiding this comment.
Reviewed 1 of 7 files at r4, 1 of 5 files at r6, 8 of 17 files at r7, all commit messages.
Reviewable status: 10 of 19 files reviewed, 10 unresolved discussions (waiting on @kungfucraig, @mariolamassaavedra, @SanjayVas, and @stevenwarejones)
src/main/proto/wfa/measurement/reporting/v2alpha/basic_reports_service.proto line 57 at r7 (raw file):
// Request mesage for 'GetListBasicReport' method message ListBasicReportsRequest {
Realized we should add a parent field for the MeasurementConsumer
There was a problem hiding this comment.
Reviewed 5 of 17 files at r7.
Reviewable status: 15 of 19 files reviewed, 10 unresolved discussions (waiting on @kungfucraig, @mariolamassaavedra, @SanjayVas, and @stevenwarejones)
There was a problem hiding this comment.
Reviewed 17 of 17 files at r7, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @kungfucraig, @mariolamassaavedra, and @stevenwarejones)
src/main/proto/wfa/measurement/reporting/v2alpha/basic_reports_service.proto line 62 at r7 (raw file):
// Return BasicReports that were created after this time. google.protobuf.Timestamp created_after = 1 [(google.api.field_behavior) = REQUIRED];
nit: It seems a bit odd to say that this field is required. I know that currently Filter only has one field, therefore if it's specified technically that one field is required. We generally expect to be able to add more fields to Filter, so I think you can just a document that at least one child field must be set.
src/main/proto/wfa/measurement/reporting/v2alpha/impression_qualification_filter_spec.proto line 34 at r7 (raw file):
// // Generally, a Filter with a given Filter Type is applied to a Reporting Unit // when the type matches that of the Reporting Media Type. However, a filer
nit
Suggestion:
filtersrc/main/proto/wfa/measurement/reporting/v2alpha/impression_qualification_filter_spec.proto line 73 at r7 (raw file):
// Specify a percent as a fraction or as a quantile message Percent { oneof value {
nit: document the oneof as required. You can't use a field behavior annotation for this, so it is purely documentation to indicate that one of the fields must be set.
src/main/proto/wfa/measurement/reporting/v2alpha/impression_qualification_filters_service.proto line 30 at r7 (raw file):
// Service for managing 'BasicReport' resources. service BasicReports {
This whole file appears to just be a copy of basic_reports_service.proto rather than defining a service for ImpressionQualificationFilter resources.
Suggestion:
ImpressionQualificationFilterssrc/main/proto/wfa/measurement/reporting/v2alpha/reporting_unit.proto line 32 at r7 (raw file):
// For use with BasicReport these must be Data Provider resource names // For use with AdvancedReport these must be Reporting Set resource names repeated string components = 2 [
nit: renumber fields so they are not sparse initially
Code quote:
2src/main/proto/wfa/measurement/reporting/v2alpha/reporting_unit.proto line 60 at r7 (raw file):
// ReportingMediaType is used to designate how reporting artifacts // should be interpreted for reporting. enum ReportingMediaType {
Why does this need to be prefixed with "Reporting"? You don't need to worry about conflicting with symbols from other APIs/packages.
Suggestion:
MediaType
There was a problem hiding this comment.
Reviewable status: 11 of 19 files reviewed, 7 unresolved discussions (waiting on @mariolamassaavedra, @SanjayVas, @stevenwarejones, and @tristanvuong2021)
src/main/proto/wfa/measurement/reporting/v2alpha/basic_report.proto line 42 at r6 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
The
namefield should only have the IDENTIFIER behavior. This inherently implies immutability after creation. The subtlety here is that the name field isn't specified in Create requests.
Done.
src/main/proto/wfa/measurement/reporting/v2alpha/basic_reports_service.proto line 57 at r7 (raw file):
Previously, tristanvuong2021 (Tristan Vuong) wrote…
Realized we should add a parent field for the MeasurementConsumer
Done.
src/main/proto/wfa/measurement/reporting/v2alpha/basic_reports_service.proto line 62 at r7 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
nit: It seems a bit odd to say that this field is required. I know that currently Filter only has one field, therefore if it's specified technically that one field is required. We generally expect to be able to add more fields to Filter, so I think you can just a document that at least one child field must be set.
If/when we add the additional field we should tag this optional and document it as you suggest.
https://google.aip.dev/203#backwards-compatibility
src/main/proto/wfa/measurement/reporting/v2alpha/impression_qualification_filters_service.proto line 30 at r7 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
This whole file appears to just be a copy of
basic_reports_service.protorather than defining a service for ImpressionQualificationFilter resources.
Sorry about that. It wasn't supposed to make the last revision, but it's updated now.
src/main/proto/wfa/measurement/reporting/v2alpha/reporting_unit.proto line 60 at r7 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
Why does this need to be prefixed with "Reporting"? You don't need to worry about conflicting with symbols from other APIs/packages.
It doesn't need to be. The disambiguation seemed like it could be useful for conversation.
kungfucraig
force-pushed
the
kungfucraig/basicreport
branch
from
d067736 to
add5182
Compare
There was a problem hiding this comment.
Reviewed 15 of 15 files at r8, all commit messages.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @kungfucraig, @mariolamassaavedra, @stevenwarejones, and @tristanvuong2021)
src/main/proto/wfa/measurement/reporting/v2alpha/impression_qualification_filters_service.proto line 36 at r8 (raw file):
returns (ImpressionQualificationFilter) { option (google.api.http) = { get: "/v2alpha/{name=impressionQualificationFilters/*}"
Suggestion:
"/v2alpha/{name=measurementConsumers/*/impressionQualificationFilters/*}"src/main/proto/wfa/measurement/reporting/v2alpha/impression_qualification_filters_service.proto line 41 at r8 (raw file):
} // Ordered by filter type and name in ascending order.
The first line needs to be summary fragment describing the method.
Suggestion:
// Lists `ImpressionQualification` resources ordered by filter type and name in ascending order.src/main/proto/wfa/measurement/reporting/v2alpha/impression_qualification_filters_service.proto line 52 at r8 (raw file):
} // Request mesage for 'GetImpressionQualificationFilter' method
nit: typo here and elsewhere
Suggestion:
messagesrc/main/proto/wfa/measurement/reporting/v2alpha/impression_qualification_filters_service.proto line 65 at r8 (raw file):
// Request mesage for 'ListImpressionQualificationFilter' method message ListImpressionQualificationFiltersRequest { // Message for specifying filters for listing ImpressionQualificationFilters.
nit: you should have the same phrasing we do on structured filter message types in the CMMS API
Suggestion:
// Filter criteria. Repeated fields are treated as logical ORs, and multiple
// fields specified as logical ANDs.src/main/proto/wfa/measurement/reporting/v2alpha/impression_qualification_filters_service.proto line 69 at r8 (raw file):
// At least one of the following criteria must be specified. message Filter { // Returns impression qualification filters with the given filter type.
nit: Verb phrases are for methods, not fields (this shouldn't start with "Returns")
Suggestion:
// Filter matching the `filter_type` field.src/main/proto/wfa/measurement/reporting/v2alpha/basic_reports_service.proto line 61 at r8 (raw file):
} // Request mesage for 'ListBasicReport' method
nit: backticks for symbols, not single quotes. Also typos
Suggestion:
// Request message for `ListBasicReports` methodsrc/main/proto/wfa/measurement/reporting/v2alpha/basic_reports_service.proto line 63 at r8 (raw file):
// Request mesage for 'ListBasicReport' method message ListBasicReportsRequest { // List the Basic Reports of the given parent.
nit: describe the field, not the action
Suggestion:
// Resource name of the parent `MeasurementConsumer`.src/main/proto/wfa/measurement/reporting/v2alpha/basic_reports_service.proto line 68 at r8 (raw file):
(google.api.resource_reference) = { type: "reporting.halo-cmm.org/MeasurementConsumer" }
You use child_type for the parent field. See https://google.aip.dev/132
I suppose using type is also fine if there's only one valid parent, but then it should be halo.wfanet.org/MeasurementConsumer since the parent is a MeasurementConsumer from the CMMS API (Reporting doesn't have its own MeasurementConsumer resource type).
Suggestion:
(google.api.resource_reference) = {
child_type: "reporting.halo-cmm.org/BasicReport"
}src/main/proto/wfa/measurement/reporting/v2alpha/basic_reports_service.proto line 77 at r8 (raw file):
// -- api-linter: core::0142::time-field-names=disabled // aip.dev/not-precedent: Using a preposition and not ending with "time" // is natural here. --)
nit: the explanation we use elsewhere is that we use structured filters instead of the AIP-160 filtering language. These inherently need relative timestamps. See https://github.com/world-federation-of-advertisers/cross-media-measurement-api/blob/main/src/main/proto/wfa/measurement/api/v2alpha/measurements_service.proto for examples
Code quote:
// aip.dev/not-precedent: Using a preposition and not ending with "time"
// is natural here. --)
kungfucraig
force-pushed
the
kungfucraig/basicreport
branch
from
ea2e849 to
757f36b
Compare
There was a problem hiding this comment.
Made several updates based upon my design work on Phase III and Advanced Report.
Reviewed 1 of 5 files at r6, 1 of 17 files at r7, 1 of 3 files at r10, 16 of 18 files at r12, 3 of 3 files at r13, 1 of 1 files at r14, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @mariolamassaavedra and @stevenwarejones)
There was a problem hiding this comment.
Reviewed 1 of 15 files at r8, 1 of 3 files at r10, 1 of 1 files at r11, 6 of 18 files at r12, 1 of 1 files at r14.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @kungfucraig and @mariolamassaavedra)
src/main/proto/wfa/measurement/reporting/v2alpha/page.proto line 66 at r11 (raw file):
CUMULATIVE = 1; // Compute non-cumulative metrics NON_CUMULATIVE = 2;
is this always a binary choice? If so, wouldn't a boolean be better?
src/main/proto/wfa/measurement/reporting/v2alpha/page.proto line 128 at r11 (raw file):
// The term that defines the filter criteria. // // May be extended to a disjunction of terms in the future.
would this comment better fit along with the existing comment on the filters field?
src/main/proto/wfa/measurement/reporting/v2alpha/page.proto line 174 at r11 (raw file):
// The k plus reach up to the frequency specified. Value must be // positive. int32 k_plus_reach = 3 [(google.api.field_behavior) = IMMUTABLE];
should this be a repeated for multiple k+ reaches? 2+, 3+, 4+ etc?
src/main/proto/wfa/measurement/reporting/v2alpha/page.proto line 179 at r11 (raw file):
// k_plus_reach, in which case if specified the percent k+ // reach values are also computed. bool percent_k_plus_reach = 4 [(google.api.field_behavior) = IMMUTABLE];
ditto
There was a problem hiding this comment.
Reviewed 1 of 3 files at r13, 1 of 1 files at r15.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @mariolamassaavedra and @stevenwarejones)
src/main/proto/wfa/measurement/reporting/v2alpha/page.proto line 66 at r11 (raw file):
Previously, stevenwarejones (Steven Ware Jones) wrote…
is this always a binary choice? If so, wouldn't a boolean be better?
It was previous a boolean. It's kind of weird as a boolean, because you'll have something called "cumulative" and it not being set means compute non-cumulative metrics. The boolean would make more sense if it mean compute cumulative metrics in addition to non-cumulative metrics.
I think this helps us keep our options open too, in the unlikely case we end up with a third value.
src/main/proto/wfa/measurement/reporting/v2alpha/page.proto line 128 at r11 (raw file):
Previously, stevenwarejones (Steven Ware Jones) wrote…
would this comment better fit along with the existing comment on the
filtersfield?
Given that this is the field that would be become repeated, I'm not so sure.
src/main/proto/wfa/measurement/reporting/v2alpha/page.proto line 174 at r11 (raw file):
Previously, stevenwarejones (Steven Ware Jones) wrote…
should this be a repeated for multiple k+ reaches? 2+, 3+, 4+ etc?
This is the field that is indicating what metrics to compute, not the values of the metrics.
If the value is 4, we compute 1+, 2+, 3+, 4+. The results structure is an array of int32.
src/main/proto/wfa/measurement/reporting/v2alpha/page.proto line 179 at r11 (raw file):
Previously, stevenwarejones (Steven Ware Jones) wrote…
ditto
Same story.
There was a problem hiding this comment.
Reviewed 1 of 5 files at r6, 1 of 17 files at r7, 8 of 18 files at r12, 2 of 3 files at r13, 1 of 1 files at r15, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @kungfucraig and @mariolamassaavedra)
src/main/proto/wfa/measurement/reporting/v2alpha/page.proto line 174 at r11 (raw file):
Previously, kungfucraig (Craig Wright) wrote…
This is the field that is indicating what metrics to compute, not the values of the metrics.
If the value is 4, we compute 1+, 2+, 3+, 4+. The results structure is an array of int32.
makes sense. I misread the comment previously. Are there scenarios where an advertiser would only want 3+ reach and nothing else?
src/main/proto/wfa/measurement/reporting/v2alpha/reporting_unit.proto line 46 at r15 (raw file):
// The MediaType filters that were applied to the components. repeated MediaType reported_media_types = 3 [
maybe I'm misunderstanding but a ReportingUnit should have results on all Media Types within a set of Event Groups preferable. Or imo, the Basic Report should strongly encourage that.
src/main/proto/wfa/measurement/reporting/v2alpha/reporting_impression_qualification_filter.proto line 36 at r15 (raw file):
// are defined as the intersection of the reporting unit's actual Media // Types and the report's Media Type filters. This is called the scoped // Media Type. For exmaple, if the Event Gruops of the reporting
Groups
Also - there is not Media Type field on Event Group yet - https://github.com/world-federation-of-advertisers/cross-media-measurement-api/blob/main/src/main/proto/wfa/measurement/api/v2alpha/event_group.proto
src/main/proto/wfa/measurement/reporting/v2alpha/page.proto line 223 at r15 (raw file):
// The impressions associated with the unique reach. // Phase III
nit: can you put TODO before all the future Phase stuff?
src/main/proto/wfa/measurement/reporting/v2alpha/page.proto line 359 at r15 (raw file):
message ResultValue { // The size of the population for the filters/grouping specified int32 population_size = 9 [(google.api.field_behavior) = IMMUTABLE];
i assume we aren't going to worry about the over 2B problem?
src/main/proto/wfa/measurement/reporting/v2alpha/page.proto line 373 at r15 (raw file):
repeated int32 k_plus_reach = 3 [(google.api.field_behavior) = IMMUTABLE]; // The perecent k+ reach
percent
src/main/proto/wfa/measurement/reporting/v2alpha/page.proto line 377 at r15 (raw file):
[(google.api.field_behavior) = IMMUTABLE]; // The frequency historam where the index is frequency-1 repeated float frequency_histogram = 5
wouldn't a map be a better structure here?
src/main/proto/wfa/measurement/reporting/v2alpha/page.proto line 417 at r15 (raw file):
// The unique reach int32 unique_rech = 2 [(google.api.field_behavior) = IMMUTABLE];
unique_reach
src/main/proto/wfa/measurement/reporting/v2alpha/impression_qualification_filter_spec.proto line 40 at r15 (raw file):
// Specifies a quantile enum Quantile {
is this part of the MRC standard?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @mariolamassaavedra and @stevenwarejones)
src/main/proto/wfa/measurement/reporting/v2alpha/impression_qualification_filter_spec.proto line 40 at r15 (raw file):
Previously, stevenwarejones (Steven Ware Jones) wrote…
is this part of the MRC standard?
What do you mean?
I'd view having quantile as just syntactic sugar over specifying it as a fraction in the oneof below.
src/main/proto/wfa/measurement/reporting/v2alpha/page.proto line 66 at r11 (raw file):
Previously, kungfucraig (Craig Wright) wrote…
It was previous a boolean. It's kind of weird as a boolean, because you'll have something called "cumulative" and it not being set means compute non-cumulative metrics. The boolean would make more sense if it mean compute cumulative metrics in addition to non-cumulative metrics.
I think this helps us keep our options open too, in the unlikely case we end up with a third value.
It also requires that the user choose which they want vs. accepting a default.
src/main/proto/wfa/measurement/reporting/v2alpha/page.proto line 174 at r11 (raw file):
Previously, stevenwarejones (Steven Ware Jones) wrote…
makes sense. I misread the comment previously. Are there scenarios where an advertiser would only want 3+ reach and nothing else?
Maybe, but the complexity of the interface that is engendered by that doesn't seem worth it, especially since the system will compute the frequency histogram from 1 to 3 as a by product anyway.
We are also moving toward precomputing a suite (i.e. a complete data cube) of metrics vs. requesting them one by one. See https://reviewable.io/reviews/world-federation-of-advertisers/cross-media-measurement/1988
src/main/proto/wfa/measurement/reporting/v2alpha/page.proto line 223 at r15 (raw file):
Previously, stevenwarejones (Steven Ware Jones) wrote…
nit: can you put TODO before all the future Phase stuff?
I'll add a TODO and comment the fields out. Sound good?
src/main/proto/wfa/measurement/reporting/v2alpha/page.proto line 359 at r15 (raw file):
Previously, stevenwarejones (Steven Ware Jones) wrote…
i assume we aren't going to worry about the over 2B problem?
I can make it an int64 if you want, but a lot of other things will break before this does.
src/main/proto/wfa/measurement/reporting/v2alpha/page.proto line 377 at r15 (raw file):
Previously, stevenwarejones (Steven Ware Jones) wrote…
wouldn't a map be a better structure here?
I'd say no. It's harder to work with and introduces the failure mode of a key not being present.
src/main/proto/wfa/measurement/reporting/v2alpha/reporting_impression_qualification_filter.proto line 36 at r15 (raw file):
Previously, stevenwarejones (Steven Ware Jones) wrote…
GroupsAlso - there is not Media Type field on Event Group yet - https://github.com/world-federation-of-advertisers/cross-media-measurement-api/blob/main/src/main/proto/wfa/measurement/api/v2alpha/event_group.proto
Not yet, but there will be.
src/main/proto/wfa/measurement/reporting/v2alpha/reporting_unit.proto line 46 at r15 (raw file):
Previously, stevenwarejones (Steven Ware Jones) wrote…
maybe I'm misunderstanding but a ReportingUnit should have results on all Media Types within a set of Event Groups preferable. Or imo, the Basic Report should strongly encourage that.
The Reporting Unit may not have results for all media types. Two examples:
- If a user creates a video only report, only results for video will be present.
- When the underlying event groups don't contain events for all media types
See the "MediaType" filter on basic report and the comments on Reporting Impressing Qualification Filter.
There was a problem hiding this comment.
Reviewed 6 of 18 files at r12, 1 of 3 files at r13, 1 of 1 files at r15, all commit messages.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @kungfucraig, @mariolamassaavedra, and @stevenwarejones)
src/main/proto/wfa/measurement/reporting/v2alpha/impression_qualification_filter.proto line 29 at r15 (raw file):
// Resource representing an Impression Qualification Filter // // An ImpressionQualificationFilter (IRF) is a named collection of
IQF?
Code quote:
IRFsrc/main/proto/wfa/measurement/reporting/v2alpha/page.proto line 30 at r15 (raw file):
option java_outer_classname = "PageProto"; // Defines the windowing criteria for a set of Metrics including the time
nit: Describe what the symbol is, not what it does
Suggestion:
Specification ofsrc/main/proto/wfa/measurement/reporting/v2alpha/page.proto line 475 at r15 (raw file):
// // This is either the resource name of an ImpressionQualificationResource or // the string "custom" indicating that custom filters were applied.
Don't use a map type for this. Resource names fields need resource reference annotations. This means breaking it out into a repeated message with key and value fields so you can annotate the key field.
Split out the "custom" one to a separate optional field rather than using a magic/sentinel string.
Code quote:
// This is either the resource name of an ImpressionQualificationResource or
// the string "custom" indicating that custom filters were applied.src/main/proto/wfa/measurement/reporting/v2alpha/metric_frequency_spec.proto line 34 at r15 (raw file):
// report partial weeks will reported for the first and/or last value // in the result set. message Weekly {
Why is this a separate message type? Why not
// The day of the week that metrics should be calculated with respect to. If not specified, metrics will not be calculated on a weekly basis.
google.type.DayOfWeek weekly_start_day = 1;src/main/proto/wfa/measurement/reporting/v2alpha/metric_frequency_spec.proto line 43 at r15 (raw file):
// Report metrics weekly Weekly weekly = 1 [(google.api.field_behavior) = IMMUTABLE]; // Report metrics across the entire reporting_interval
nit: The summary fragment should say what the symbol is, not what it does.
Suggestion:
Whether to report metrics across the entire `reporting_interval`.src/main/proto/wfa/measurement/reporting/v2alpha/basic_report.proto line 65 at r15 (raw file):
// The set of Media Types to filter the campaign_group on. // // These are applied as to the campaign group disjunctively.
nit: I don't understand this statement
src/main/proto/wfa/measurement/reporting/v2alpha/basic_report.proto line 87 at r15 (raw file):
]; // The results for each Page Definitions in the same order as the definitions.
Is this supposed to be referring to page_specs? If so, reference the field.
Suggestion:
// The result for each item in `page_specs` in the same order.
There was a problem hiding this comment.
Reviewed 4 of 6 files at r16.
Reviewable status: 18 of 21 files reviewed, 10 unresolved discussions (waiting on @mariolamassaavedra, @SanjayVas, @stevenwarejones, and @tristanvuong2021)
src/main/proto/wfa/measurement/reporting/v2alpha/basic_report.proto line 87 at r15 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
Is this supposed to be referring to
page_specs? If so, reference the field.
Done.
src/main/proto/wfa/measurement/reporting/v2alpha/impression_qualification_filter.proto line 29 at r15 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
IQF?
Done.
src/main/proto/wfa/measurement/reporting/v2alpha/metric_frequency_spec.proto line 34 at r15 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
Why is this a separate message type? Why not
// The day of the week that metrics should be calculated with respect to. If not specified, metrics will not be calculated on a weekly basis. google.type.DayOfWeek weekly_start_day = 1;
The only reason to have it as a separate message to to provide flexibility in the future. I could go either way on this. If you feel strongly about it I'll change it.
src/main/proto/wfa/measurement/reporting/v2alpha/page.proto line 475 at r15 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
Don't use a
maptype for this. Resource names fields need resource reference annotations. This means breaking it out into a repeated message withkeyandvaluefields so you can annotate thekeyfield.Split out the "custom" one to a separate optional field rather than using a magic/sentinel string.
Done.
There was a problem hiding this comment.
Reviewed 2 of 18 files at r12, 1 of 1 files at r14, 3 of 6 files at r16, 1 of 1 files at r17, all commit messages.
Reviewable status: 19 of 21 files reviewed, 10 unresolved discussions (waiting on @kungfucraig, @mariolamassaavedra, @stevenwarejones, and @tristanvuong2021)
src/main/proto/wfa/measurement/reporting/v2alpha/metric_frequency_spec.proto line 34 at r15 (raw file):
Previously, kungfucraig (Craig Wright) wrote…
The only reason to have it as a separate message to to provide flexibility in the future. I could go either way on this. If you feel strongly about it I'll change it.
Depends on how likely you think this is to change, just like any other field. If you don't think it's likely, then avoid speculative code. Either way, the documentation needs to be updated a bit. Specifically, since this isn't a boolean field then you need to instead document what the field is and what happens when it's not specified.
src/main/proto/wfa/measurement/reporting/v2alpha/page.proto line 474 at r17 (raw file):
// that was applied when computing it. message PageAndFilter { // The filter. Required.
Did you want this to behave like a map, i.e. that you don't have duplicate keys? If so, I'd recommend both documenting that and also changing the structure a bit to make it more clear.
// Entry for the `pages` map.
message PagesMapEntry {
// Map entry key.
string key = 1 [
(google.api.resource_reference) = {
type: "reporting.halo-cmm.org/ImpressionQualificationFilter"
},
(google.api.field_behavior) = IMMUTABLE,
(google.api.field_behavior) = REQUIRED
];
// Map entry value.
Page value = 2 [(google.api.field_behavior) = IMMUTABLE, (google.api.field_behavior) = REQUIRED];
}
// Map of `ImpressionQualificationFilter` to `Page`.
repeated PagesMapEntry pages = 1 [(google.api.field_behavior) = IMMUTABLE];
// The `Page` entry for a custom filter.
Page custom_filter_page = 2 [(google.api.field_behavior) = IMMUTABLE, (google.api.field_behavior) = OPTIONAL];src/main/proto/wfa/measurement/reporting/v2alpha/basic_report.proto line 67 at r17 (raw file):
repeated MediaType media_types = 5 [ (google.api.field_behavior) = REQUIRED, (google.api.field_behavior) = IMMUTABLE
The comment describes this as a set. If this means that the order also doesn't matter, add the UNORDERED_LIST field behavior.
src/main/proto/wfa/measurement/reporting/v2alpha/impression_qualification_filters_service.proto line 69 at r17 (raw file):
// If unspecified, at most 50 filters will be returned. // The maximum value is 100; values above 100 will be coerced to 100. int32 page_size = 2 [(google.api.field_behavior) = OPTIONAL];
nit: collapse field tag numbers
There was a problem hiding this comment.
Reviewed 2 of 6 files at r16, 3 of 4 files at r18, 1 of 1 files at r19.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @mariolamassaavedra, @SanjayVas, @stevenwarejones, and @tristanvuong2021)
src/main/proto/wfa/measurement/reporting/v2alpha/basic_report.proto line 67 at r17 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
The comment describes this as a set. If this means that the order also doesn't matter, add the
UNORDERED_LISTfield behavior.
Done.
src/main/proto/wfa/measurement/reporting/v2alpha/metric_frequency_spec.proto line 34 at r15 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
Depends on how likely you think this is to change, just like any other field. If you don't think it's likely, then avoid speculative code. Either way, the documentation needs to be updated a bit. Specifically, since this isn't a boolean field then you need to instead document what the field is and what happens when it's not specified.
Removed it.
src/main/proto/wfa/measurement/reporting/v2alpha/page.proto line 474 at r17 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
Did you want this to behave like a map, i.e. that you don't have duplicate keys? If so, I'd recommend both documenting that and also changing the structure a bit to make it more clear.
// Entry for the `pages` map. message PagesMapEntry { // Map entry key. string key = 1 [ (google.api.resource_reference) = { type: "reporting.halo-cmm.org/ImpressionQualificationFilter" }, (google.api.field_behavior) = IMMUTABLE, (google.api.field_behavior) = REQUIRED ]; // Map entry value. Page value = 2 [(google.api.field_behavior) = IMMUTABLE, (google.api.field_behavior) = REQUIRED]; } // Map of `ImpressionQualificationFilter` to `Page`. repeated PagesMapEntry pages = 1 [(google.api.field_behavior) = IMMUTABLE]; // The `Page` entry for a custom filter. Page custom_filter_page = 2 [(google.api.field_behavior) = IMMUTABLE, (google.api.field_behavior) = OPTIONAL];
We can do it that way, but I'm not convinced this is an improvement over the use of map<string, Page> with the sentinel value and no annotation.
There was a problem hiding this comment.
Reviewed 5 of 18 files at r12, 1 of 3 files at r13, 2 of 6 files at r16, 3 of 4 files at r18, 1 of 1 files at r19.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @kungfucraig, @mariolamassaavedra, @stevenwarejones, and @tristanvuong2021)
src/main/proto/wfa/measurement/reporting/v2alpha/page.proto line 474 at r17 (raw file):
I'm not convinced this is an improvement over the use of map<string, Page> with the sentinel value and no annotation.
The use of the sentinel value and the lack of annotation are, in decreasing order of importance, the precise factors that make that option unviable.
src/main/proto/wfa/measurement/reporting/v2alpha/reporting_impression_qualification_filter.proto line 62 at r19 (raw file):
// Specifies which filter to use. Required. oneof filter { // Specifies an ImpressionQualificationFilter.
nit: the old comment was better, as again the summary fragment of a type or field should not be a verb phrase. The only place a verb phrase should appear in a summary fragment documenting a symbol in a protobuf API definition is on a method.
This means that starting a field or message documentation with "Specifies" or "Defines" is never correct.
src/main/proto/wfa/measurement/reporting/v2alpha/reporting_interval.proto line 28 at r19 (raw file):
// The time interval over which Metrics are generated. This interval is // closed on both sides.
nit: the interval isn't actually closed on both sides, it's just that the end is specified as an inclusive date despite the interval being over time. It's actually forming an interval of [start time, end time).
It may be clearer to describe it this way rather than saying that the end date is inclusive w.r.t. date but exclusive w.r.t. time.
Code quote:
// The time interval over which Metrics are generated. This interval is
// closed on both sides.
There was a problem hiding this comment.
Reviewed 3 of 6 files at r16, 3 of 4 files at r18, 1 of 1 files at r19, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @kungfucraig, @mariolamassaavedra, and @tristanvuong2021)
src/main/proto/wfa/measurement/reporting/v2alpha/impression_qualification_filter_spec.proto line 40 at r15 (raw file):
Previously, kungfucraig (Craig Wright) wrote…
What do you mean?
I'd view having quantile as just syntactic sugar over specifying it as a fraction in the oneof below.
Why quintile over quartiles? just wondering if one or the other is a formal/informal standard.
I see more references to using deciles or quartiles for completion. I can't find any references for using segments for viewability.
src/main/proto/wfa/measurement/reporting/v2alpha/impression_qualification_filter_spec.proto line 66 at r19 (raw file):
} // Specifies the viewpability filter
This is a little confusing to me how viewability percent works. Viewability depends on two dimensions: % viewed and time.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @kungfucraig, @mariolamassaavedra, and @tristanvuong2021)
src/main/proto/wfa/measurement/reporting/v2alpha/reporting_unit.proto line 46 at r15 (raw file):
Previously, kungfucraig (Craig Wright) wrote…
The Reporting Unit may not have results for all media types. Two examples:
- If a user creates a video only report, only results for video will be present.
- When the underlying event groups don't contain events for all media types
See the "MediaType" filter on basic report and the comments on Reporting Impressing Qualification Filter.
Its my understanding that a Basic Report will need to include data from all the media types of the included Event Groups. If the user wants results from only one Media Type:
- We'd need to flag errors if one of the included Event Groups does not contain that Media Type
- Possibly only allow Media Type to be filtered out outside the Basic Report. wdyt?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @kungfucraig, @mariolamassaavedra, and @tristanvuong2021)
src/main/proto/wfa/measurement/reporting/v2alpha/reporting_unit.proto line 46 at r15 (raw file):
Previously, stevenwarejones (Steven Ware Jones) wrote…
Its my understanding that a Basic Report will need to include data from all the media types of the included Event Groups. If the user wants results from only one Media Type:
- We'd need to flag errors if one of the included Event Groups does not contain that Media Type
- Possibly only allow Media Type to be filtered out outside the Basic Report. wdyt?
Media Type filtering seems more like an Advanced Report feature to me.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r20, 1 of 2 files at r21.
Reviewable status: 20 of 21 files reviewed, 5 unresolved discussions (waiting on @mariolamassaavedra, @SanjayVas, @stevenwarejones, and @tristanvuong2021)
src/main/proto/wfa/measurement/reporting/v2alpha/impression_qualification_filter_spec.proto line 40 at r15 (raw file):
Previously, stevenwarejones (Steven Ware Jones) wrote…
Why quintile over quartiles? just wondering if one or the other is a formal/informal standard.
I see more references to using deciles or quartiles for completion. I can't find any references for using segments for viewability.
These map to the Origin event template for percent completion: https://github.com/world-federation-of-advertisers/uk-pilot-event-templates/blob/main/src/main/proto/halo_cmm/uk/pilot/video_template.proto
It's unrelated to the PR, but using booleans instead of floats for those event templates needs to be discussed.
src/main/proto/wfa/measurement/reporting/v2alpha/impression_qualification_filter_spec.proto line 66 at r19 (raw file):
Previously, stevenwarejones (Steven Ware Jones) wrote…
This is a little confusing to me how viewability percent works. Viewability depends on two dimensions: % viewed and time.
Let's discuss this on Monday. I think we need to improve the event templates. See: https://github.com/world-federation-of-advertisers/uk-pilot-event-templates/blob/main/src/main/proto/halo_cmm/uk/pilot/display_template.proto#L24
src/main/proto/wfa/measurement/reporting/v2alpha/reporting_interval.proto line 28 at r19 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
nit: the interval isn't actually closed on both sides, it's just that the end is specified as an inclusive date despite the interval being over time. It's actually forming an interval of
[start time, end time).It may be clearer to describe it this way rather than saying that the end date is inclusive w.r.t. date but exclusive w.r.t. time.
Decided to drop the second sentence. The documentation of the individual fields is adequate to describe what is going on.
src/main/proto/wfa/measurement/reporting/v2alpha/reporting_unit.proto line 46 at r15 (raw file):
Previously, stevenwarejones (Steven Ware Jones) wrote…
Media Type filtering seems more like an Advanced Report feature to me.
Let's discuss Monday. I'm open to dropping it.
No description provided.