Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add Create method to BasicReport #1994

Open
wants to merge 36 commits into
base: main
Choose a base branch
from

Conversation

kungfucraig
Copy link
Member

@kungfucraig kungfucraig commented Jan 8, 2025

Changes to the MC API required for Phase II.

@wfa-reviewable
Copy link

This change is Reviewable

@kungfucraig kungfucraig changed the base branch from main to kungfucraig/mcapiphaseone January 8, 2025 18:16
@kungfucraig kungfucraig changed the title feat: Add protos for MC API Phase II feat: Add Create method to BasicReport Jan 8, 2025
@kungfucraig kungfucraig force-pushed the kungfucraig/mcapiphasetwo branch from e0f1e3b to a919516 Compare January 9, 2025 18:26
@kungfucraig kungfucraig force-pushed the kungfucraig/mcapiphasetwo branch from ad25ae1 to 3739a5b Compare January 9, 2025 18:45
Copy link
Member Author

@kungfucraig kungfucraig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 4 files at r1, 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @kungfucraig)

@SanjayVas
Copy link
Member

Depends on #1993

Copy link
Member

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


src/main/proto/wfa/measurement/reporting/v2alpha/page.proto line 93 at r3 (raw file):

  // etc.)
  message BasicMetricSetSpec {
    // The reach

nit: describe the actual fields. The current descriptions imply that the fields contain result values. In general, I'd expect boolean fields here to start with "whether" and other fields to contain "if specified" or "if not specified"

Suggestion:

// Whether to include reach in the result

src/main/proto/wfa/measurement/reporting/v2alpha/basic_reports_service.proto line 144 at r3 (raw file):

  // The BasicReport to create.
  BasicReport basic_report = 3;

Shouldn't this also be [(google.api.field_behavior) = REQUIRED]?

@kungfucraig kungfucraig force-pushed the kungfucraig/mcapiphasetwo branch 2 times, most recently from 23637f9 to 1e07d4f Compare January 14, 2025 00:35
Copy link
Member Author

@kungfucraig kungfucraig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


src/main/proto/wfa/measurement/reporting/v2alpha/basic_reports_service.proto line 144 at r3 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Shouldn't this also be [(google.api.field_behavior) = REQUIRED]?

Done.


src/main/proto/wfa/measurement/reporting/v2alpha/page.proto line 93 at r3 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

nit: describe the actual fields. The current descriptions imply that the fields contain result values. In general, I'd expect boolean fields here to start with "whether" and other fields to contain "if specified" or "if not specified"

That adds an awful lot of noise to the documentation. What do you think of this the updates I made instead?

Copy link
Member

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 4 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kungfucraig, @mariolamassaavedra, @stevenwarejones, and @tristanvuong2021)


src/main/proto/wfa/measurement/reporting/v2alpha/page.proto line 93 at r3 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

That adds an awful lot of noise to the documentation. What do you think of this the updates I made instead?

I don't think it's terribly noisy, but this comment is also a nit

Copy link
Collaborator

@stevenwarejones stevenwarejones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 4 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @mariolamassaavedra and @tristanvuong2021)

@kungfucraig kungfucraig force-pushed the kungfucraig/mcapiphaseone branch from 8f3757c to 1432426 Compare January 16, 2025 22:02
@kungfucraig kungfucraig force-pushed the kungfucraig/mcapiphasetwo branch from aefb87d to 56fa07f Compare January 16, 2025 22:04
Base automatically changed from kungfucraig/mcapiphaseone to main January 17, 2025 16:30
@kungfucraig kungfucraig force-pushed the kungfucraig/mcapiphasetwo branch from 56fa07f to 5a3a609 Compare January 17, 2025 17:03
Copy link
Member Author

@kungfucraig kungfucraig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 4 files at r7, 1 of 2 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @mariolamassaavedra and @tristanvuong2021)

Copy link
Member

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 4 files at r7, 1 of 2 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @kungfucraig, @mariolamassaavedra, and @tristanvuong2021)


src/main/proto/wfa/measurement/reporting/v2alpha/page.proto line 62 at r9 (raw file):

  // and in the common template are supported.
  message Grouping {
    // A set field paths that indicate the dimensions to group by.

nit: Missing a word, but I think it's better to just drop the "set" since that sometimes implies unordered.

Suggestion:

Field

src/main/proto/wfa/measurement/reporting/v2alpha/page.proto line 160 at r9 (raw file):

    BasicMetricSetSpec basic = 1 [(google.api.field_behavior) = IMMUTABLE];

    // The unique reach of each item in reporting_unit.components

reporting_unit has type ReportingUnitMetricSetSpec which has no components field. Are you referring to a different reporting_unit field, such as the one in the parent PageSpec?

Code quote:

of each item in reporting_unit.components

src/main/proto/wfa/measurement/reporting/v2alpha/page.proto line 406 at r9 (raw file):

// See the definition of `ReportingImpressionQualificationFilter` for details
// on what it means for a filter to be active.
message PageResult {

Where is this used?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants