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 protos for read-only BasicReport #1993

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

Conversation

kungfucraig
Copy link
Member

No description provided.

@wfa-reviewable
Copy link

This change is Reviewable

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 9 of 19 files at r1, 6 of 6 files at r2, all commit messages.
Reviewable status: 15 of 20 files reviewed, 9 unresolved discussions (waiting on @kungfucraig, @mariolamassaavedra, @stevenwarejones, and @tristanvuong2021)


src/main/proto/wfa/measurement/reporting/v2alpha/basic_report.proto line 46 at r2 (raw file):

  string title = 2 [(google.api.field_behavior) = IMMUTABLE];

  // This is the resource name of a primitive Reporting Set that includes all of

nit: use code font when referencing something that is a known symbol, e.g. DataProvider. Apparently you can now also use cross-references.

See https://google.aip.dev/192#formatting and https://google.aip.dev/192#cross-references


src/main/proto/wfa/measurement/reporting/v2alpha/reporting_impression_qualification_filter.proto line 53 at r2 (raw file):

  message CustomImpressionQualificationFilterSpec {
    // The impression filter criteria, no more than on per MediaType
    repeated ImpressionQualificationFilterSpec filter_spec = 2 [

nit

Suggestion:

1

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

  }

  // Ordered by `create_time` ascending, `name` ascending.

nit: the summary fragment still needs to describe the method

Suggestion:

// Lists `BasicReport` resources ordered by (`create_time` ascending, `name` ascending).

src/main/proto/wfa/measurement/reporting/v2alpha/event_filter.proto line 43 at r2 (raw file):

        string string_value = 1 [(google.api.field_behavior) = IMMUTABLE];
        // Used when the event template field is an enum
        string enum_value = 2 [(google.api.field_behavior) = IMMUTABLE];

Assuming we want to use the number, as that's how enums are usually encoded over the wire.

Suggestion:

int32

src/main/proto/wfa/measurement/reporting/v2alpha/event_filter.proto line 45 at r2 (raw file):

        string enum_value = 2 [(google.api.field_behavior) = IMMUTABLE];
        // Used when the event template field is a Boolean
        string bool_value = 3 [(google.api.field_behavior) = IMMUTABLE];

Suggestion:

bool

src/main/proto/wfa/measurement/reporting/v2alpha/event_filter.proto line 54 at r2 (raw file):

    ];
  }
  // The terms that define the filter criteria.

Document that this forms a conjunction.


src/main/proto/wfa/measurement/reporting/v2alpha/event_filter.proto line 57 at r2 (raw file):

  //
  // The a current limintation is that the number of terms must be
  // exactly one.

nit

Suggestion:

  // The current limitation is that the number of terms must be exactly one.

src/main/proto/wfa/measurement/reporting/v2alpha/reporting_unit.proto line 39 at r2 (raw file):

  ];

  // Display names of the resource referenced by the Reporting Unit

Is this supposed to match up with the components field cardinality-wise? If so, maybe pull both fields into a separate message and repeat that.


src/main/proto/wfa/measurement/reporting/v2alpha/event_groups_service.proto line 21 at r2 (raw file):

import "google/api/client.proto";
import "google/api/field_behavior.proto";
import "google/api/annotations.proto";

Unrelated change? Also isn't this sorting incorrectly? I assume it's supposed to be lexicographical?

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 11 of 19 files at r1, 5 of 6 files at r2, 16 of 17 files at r3, 5 of 7 files at r4, all commit messages.
Reviewable status: 18 of 20 files reviewed, 5 unresolved discussions (waiting on @mariolamassaavedra, @SanjayVas, @stevenwarejones, and @tristanvuong2021)


src/main/proto/wfa/measurement/reporting/v2alpha/event_filter.proto line 43 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Assuming we want to use the number, as that's how enums are usually encoded over the wire.

I would rather not use the number. It's harder on the client, and either way error checking has to be done server side.

I'd be up for dropping enum_value and allowing either string_value or int_value for enum types though.


src/main/proto/wfa/measurement/reporting/v2alpha/event_filter.proto line 54 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Document that this forms a conjunction.

It's a disjunction, but yeah. A repeated Filter is a conjunction of disjunctive Terms.


src/main/proto/wfa/measurement/reporting/v2alpha/reporting_unit.proto line 39 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Is this supposed to match up with the components field cardinality-wise? If so, maybe pull both fields into a separate message and repeat that.

I need to think on this for a bit. It may make sense to separate the structure used in the Spec from the one that's used by the Result.


src/main/proto/wfa/measurement/reporting/v2alpha/event_groups_service.proto line 21 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Unrelated change? Also isn't this sorting incorrectly? I assume it's supposed to be lexicographical?

Yeah, somehow this snuck in.


src/main/proto/wfa/measurement/reporting/v2alpha/event_filter.proto line 45 at r2 (raw file):

        string enum_value = 2 [(google.api.field_behavior) = IMMUTABLE];
        // Used when the event template field is a Boolean
        string bool_value = 3 [(google.api.field_behavior) = IMMUTABLE];

Done.

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 1 of 7 files at r4, 1 of 2 files at r5.
Reviewable status: 18 of 20 files reviewed, 5 unresolved discussions (waiting on @mariolamassaavedra, @SanjayVas, @stevenwarejones, and @tristanvuong2021)

@kungfucraig kungfucraig force-pushed the kungfucraig/mcapiphaseone branch from 526e79e to f58404e Compare January 8, 2025 02:39
@kungfucraig kungfucraig changed the title feat: Add protos for read-only Basic Report feat: Add protos for read-only BasicReport Jan 8, 2025
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 1 of 6 files at r2, 6 of 17 files at r3, 2 of 7 files at r4, 1 of 2 files at r5, 7 of 7 files at r6, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @kungfucraig, @mariolamassaavedra, @SanjayVas, and @tristanvuong2021)


src/main/proto/wfa/measurement/reporting/v2alpha/event_filter.proto line 54 at r2 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

It's a disjunction, but yeah. A repeated Filter is a conjunction of disjunctive Terms.

So, the idea is to have a conjunctions of disjunctions, right?


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

    // Default value. Unused.
    ACCUMULATION_OPTIONS_UNSPECIFIED = 0;
    // Comulative metrics

nit: Cumulative


src/main/proto/wfa/measurement/reporting/v2alpha/impression_qualification_filter.proto line 33 at r6 (raw file):

// one spec per MediaType. For example, an IQF to filter all measurable
// impressions for VIDEO and DISPLAY would provide one spec for
// video with viewability and compleition percent set to zero and a

nit: completion


src/main/proto/wfa/measurement/reporting/v2alpha/impression_qualification_filter.proto line 54 at r6 (raw file):

  // Specifies the filter criteria for each MediaType.
  //
  // They may be at most one spec per MediaType.

nit: There


src/main/proto/wfa/measurement/reporting/v2alpha/impression_qualification_filter_spec.proto line 42 at r6 (raw file):

  // The filter criteria.
  //
  // Event Tempalte fields that are used by any filter Terms must be

nit: Template.

Also, these are a conjunction? If so, can you document that.

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 1 of 2 files at r5, 7 of 7 files at r6, 6 of 6 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @mariolamassaavedra, @SanjayVas, and @tristanvuong2021)


src/main/proto/wfa/measurement/reporting/v2alpha/event_filter.proto line 54 at r2 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

So, the idea is to have a conjunctions of disjunctions, right?

Yes. The repeated Filter is a conjunction. The repeated Term is a disjunction.


src/main/proto/wfa/measurement/reporting/v2alpha/reporting_unit.proto line 39 at r2 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

I need to think on this for a bit. It may make sense to separate the structure used in the Spec from the one that's used by the Result.

I split the Reporting Unit structure such that it is only used in the PageSpec and created a ReportingUnitSummary structure that is used for results. This should get the spec more expressive and allow us more flexibility on structuring output only fields.

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 5 of 6 files at r7, 1 of 1 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @mariolamassaavedra, @SanjayVas, and @tristanvuong2021)

Copy link
Contributor

@tristanvuong2021 tristanvuong2021 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 17 files at r3, 2 of 7 files at r4, 7 of 7 files at r6, 5 of 6 files at r7, 1 of 1 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @kungfucraig, @mariolamassaavedra, and @SanjayVas)


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

// Copyright 2024 The Cross-Media Measurement Authors

2025


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

// Copyright 2024 The Cross-Media Measurement Authors

2025

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.

5 participants