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

Update Benchmark and MeasurementSystem with population results #1164

Merged
merged 16 commits into from
Aug 31, 2023

Conversation

stevenwarejones
Copy link
Collaborator

@stevenwarejones stevenwarejones commented Aug 11, 2023

  • Adds population flags to Create Measurement including refactoring all Event-specific flags under EventMeasurementParams
  • Consolidates flags from MeasurementSystem and Benchmark in CreateMeasurementFlags
  • Fixes a bug in BenchmarkTest that wasn't actually returning the correct result for each measurement and was verifying the wrong thing. Note: This behavior still exists in MeasurementSystemTest but since we aren't verifying any results, this seems unimportant to fix at this time.

@wfa-reviewable
Copy link

This change is Reviewable

@stevenwarejones stevenwarejones marked this pull request as ready for review August 11, 2023 00:12
@stevenwarejones stevenwarejones changed the base branch from stevenwarejones_population_service_cmapi_1 to main August 11, 2023 00:12
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.

Thanks for the refactoring and fixes!

@renjiezh for changes to Benchmark

Reviewed 24 of 24 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @renjiezh and @stevenwarejones)


src/main/kotlin/org/wfanet/measurement/api/v2alpha/tools/BUILD.bazel line 30 at r2 (raw file):

    name = "measurement_system",
    srcs = [
        "CreateMeasurementFlags.kt",

Source files should not be referenced by more than one library target. Create a separate kt_jvm_library for this source file and add it to the deps of both measurement_system and benchmark_library.

Code quote:

 "CreateMeasurementFlags.kt",

Copy link
Contributor

@renjiezh renjiezh left a comment

Choose a reason for hiding this comment

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

FYI, there are 2 PRs having merging conflict with this one.
#1188
#1183

Reviewed 24 of 24 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @stevenwarejones)


src/test/kotlin/org/wfanet/measurement/api/v2alpha/tools/MeasurementSystemTest.kt line 1984 at r2 (raw file):

        state = Measurement.State.SUCCEEDED

        results += resultPair {

Could you add a TODO here to remind me to merge results into one object.

Copy link
Collaborator Author

@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: all files reviewed, 1 unresolved discussion (waiting on @SanjayVas)


src/main/kotlin/org/wfanet/measurement/api/v2alpha/tools/BUILD.bazel line 30 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Source files should not be referenced by more than one library target. Create a separate kt_jvm_library for this source file and add it to the deps of both measurement_system and benchmark_library.

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 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @stevenwarejones)


src/main/kotlin/org/wfanet/measurement/api/v2alpha/tools/CreateMeasurementFlags.kt line 92 at r3 (raw file):

        )
        lateinit var name: String
          set

These should all have private set as they are only set by Picocli CommandLine injection.


src/test/kotlin/org/wfanet/measurement/api/v2alpha/tools/MeasurementSystemTest.kt line 1978 at r3 (raw file):

    }

    // TODO: Have separate successful measurements for each measurement type

Use KDoc syntax for properties. See https://developer.android.com/kotlin/style-guide#documentation

If possible, indicate a context for TODOs. See https://github.com/world-federation-of-advertisers/cross-media-measurement/blob/main/docs/code-style.md#todos

Copy link
Collaborator Author

@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: all files reviewed, 2 unresolved discussions (waiting on @SanjayVas)


src/main/kotlin/org/wfanet/measurement/api/v2alpha/tools/CreateMeasurementFlags.kt line 92 at r3 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

These should all have private set as they are only set by Picocli CommandLine injection.

Done.


src/test/kotlin/org/wfanet/measurement/api/v2alpha/tools/MeasurementSystemTest.kt line 1978 at r3 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Use KDoc syntax for properties. See https://developer.android.com/kotlin/style-guide#documentation

If possible, indicate a context for TODOs. See https://github.com/world-federation-of-advertisers/cross-media-measurement/blob/main/docs/code-style.md#todos

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 2 of 2 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @stevenwarejones)

@stevenwarejones stevenwarejones merged commit 9e6ae04 into main Aug 31, 2023
@stevenwarejones stevenwarejones deleted the stevenwarejones_population_service_cmapi_2 branch August 31, 2023 01:52
ple13 pushed a commit that referenced this pull request Aug 16, 2024
* Adds population flags to Create Measurement including refactoring all
Event-specific flags under EventMeasurementParams
* Consolidates flags from MeasurementSystem and Benchmark in
CreateMeasurementFlags
* Fixes a bug in BenchmarkTest that wasn't actually returning the
correct result for each measurement and was verifying the wrong thing.
Note: This behavior still exists in MeasurementSystemTest but since we
aren't verifying any results, this seems unimportant to fix at this
time.
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