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 ReportMetadata to MeasurementSpec #222

Open
wants to merge 8 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

@kungfucraig kungfucraig requested review from SanjayVas, jojijac0b and stevenwarejones and removed request for jojijac0b December 19, 2024 23:56
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.

Make sure you don't merge this until you have the corresponding PR in the main repo ready.

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @kungfucraig and @stevenwarejones)


src/main/proto/wfa/measurement/api/v2alpha/measurement_spec.proto line 148 at r2 (raw file):

  ];

  // Provides the report and metric for which this Measurement was created

nit: Verb phrases are for methods, not fields.

Suggestion:

  // Metadata about the use of this `Measurement` in the Reporting system.

src/main/proto/wfa/measurement/api/v2alpha/measurement_spec.proto line 149 at r2 (raw file):

  // Provides the report and metric for which this Measurement was created
  message ReportMetadata {

nit: Maybe rename for flexibility, since this is arguably just metadata from the Reporting system

Suggestion:

ReportingMetadata

src/main/proto/wfa/measurement/api/v2alpha/measurement_spec.proto line 150 at r2 (raw file):

  // Provides the report and metric for which this Measurement was created
  message ReportMetadata {
    // The Report

nit

Suggestion:

// Resource name of the `Report`.

Copy link
Contributor

@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 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @jojijac0b and @kungfucraig)


src/main/proto/wfa/measurement/api/v2alpha/measurement_spec.proto line 155 at r3 (raw file):

      (google.api.field_behavior) = REQUIRED,
      (google.api.field_behavior) = IMMUTABLE,
      (google.api.resource_reference) = { type: "halo.wfanet.org/Report" }

what's the right way to inform that this is halo.wfanet.org.reporting/Report

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.

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @jojijac0b, @kungfucraig, and @stevenwarejones)


src/main/proto/wfa/measurement/api/v2alpha/measurement_spec.proto line 155 at r3 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

what's the right way to inform that this is halo.wfanet.org.reporting/Report

This is the right way to document the type, but I missed that the type listed here is actually incorrect. It should be reporting.halo-cmm.org/Report

Similarly, the Reporting metric type is `reporting.halo-cmm.org/Metric

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.

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @jojijac0b, @SanjayVas, and @stevenwarejones)


src/main/proto/wfa/measurement/api/v2alpha/measurement_spec.proto line 155 at r3 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

This is the right way to document the type, but I missed that the type listed here is actually incorrect. It should be reporting.halo-cmm.org/Report

Similarly, the Reporting metric type is `reporting.halo-cmm.org/Metric

Done.

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 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jojijac0b and @stevenwarejones)

Copy link
Contributor

@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 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jojijac0b and @kungfucraig)


src/main/proto/wfa/measurement/api/v2alpha/measurement_spec.proto line 155 at r3 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

Done.

this addresses the concern I had

Copy link
Contributor

@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.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jojijac0b)

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