Provide --experimental_instrumentation_filter_fragment cli option
#15
Conversation
This option is similar to `--instrumentation_filter` option. The difference is this option can be used multiple times to accumulate its values. If this option is provided `--instrumentation_filter` has no effect. Multiple uses of `--instrumentation_filter` are not accumulated. This makes it difficult to have short hand configuration flags for a combination of coverage flags in a bazelrc file. e.g., ``` build:covA --instrumentation_filter=afoo/abar,-afoo/abaz build:covB --instrumentation_filter=bfoo/bbar,-bfoo/bbaz build:covC --instrumentation_filter=cfoo/cbar,-cfoo/cbaz \# To combine the flags concatenate the respective filter strings build:covAB --instrumentation_filter=afoo/abar,-afoo/abaz,bfoo/bbar,-bfoo/bbaz build:covAC --instrumentation_filter=afoo/abar,-afoo/abaz,cfoo/cbar,-cfoo/cbaz build:covABC --instrumentation_filter=afoo/abar,-afoo/abaz,bfoo/bbar,-bfoo/bbaz,cfoo/cbar,-cfoo/cbaz ``` With a large set of flags and their respective large filter strings it is uneasy to combine the short hand configuration flags because the respective filter strings need to be concatenated into a single string. This is uneasy to maintain because filter strings are repeated in the combinations. `--experimental_instrumentation_filter_fragment` simplifies combining multiple shorthand configuration flags because its multiple uses are accumulated. e.g., ``` build:covA --experimental_instrumentation_filter_fragment=afoo/abar,-afoo/abaz build:covB --experimental_instrumentation_filter_fragment=bfoo/bbar,-bfoo/bbaz build:covC --experimental_instrumentation_filter_fragment=cfoo/cbar,-cfoo/cbaz build:covAB --config covA --config covB build:covAC --config covA --config covC build:covABC --config covA --config covB --config covC ``` Add tests for --experimental_instrumentation_filter_fragment cli to verify that its multiple uses are accumulated and --instrumentation_filter has no effect when --experimental_instrumentation_filter_fragment is used. bazelbuild#22959 is the upstream issue. Testing Done: ``` $ bazelisk test src/test/java/com/google/devtools/build/lib/starlark:all INFO: Analyzed 4 targets (0 packages loaded, 0 targets configured). INFO: Found 1 target and 3 test targets... INFO: Elapsed time: 242.197s, Critical Path: 216.72s INFO: 27 processes: 1 internal, 25 darwin-sandbox, 1 worker. INFO: Build completed successfully, 27 total actions //src/test/java/com/google/devtools/build/lib/starlark:BindTest (cached) PASSED in 7.5s //src/test/java/com/google/devtools/build/lib/starlark:StarlarkRuleClassFunctionsTest (cached) PASSED in 78.0s Stats over 5 runs: max = 78.0s, min = 59.8s, avg = 73.9s, dev = 7.1s //src/test/java/com/google/devtools/build/lib/starlark:StarlarkTests PASSED in 121.1s Stats over 25 runs: max = 121.1s, min = 24.8s, avg = 88.6s, dev = 32.1s Executed 1 out of 3 tests: 3 tests pass. There were tests whose specified size is too big. Use the --test_verbose_timeout_warnings command line option to see which ones these are. ```
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
As I commented earlier, I still think But otherwise, I don't mind accepting this as an experimental feature. |
WalkthroughThis change introduces a new experimental command-line option, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CoreOptions
participant BuildConfigurationValue
participant RegexFilter
User->>CoreOptions: Specify --experimental_instrumentation_filter_fragment (possibly multiple times)
CoreOptions->>BuildConfigurationValue: Pass options to constructor
BuildConfigurationValue->>BuildConfigurationValue: Check if instrumentationFilterFragment is non-empty
alt instrumentationFilterFragment is non-empty
BuildConfigurationValue->>RegexFilter: Convert joined fragments to RegexFilter
RegexFilter-->>BuildConfigurationValue: Return RegexFilter instance
BuildConfigurationValue->>BuildConfigurationValue: Set instrumentationFilter to new RegexFilter
else instrumentationFilterFragment is empty
BuildConfigurationValue->>BuildConfigurationValue: Set instrumentationFilter to legacy option
end
BuildConfigurationValue->>User: getInstrumentationFilter() returns the correct filter
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @arvi18, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request introduces a new command-line option, --experimental_instrumentation_filter_fragment, to accumulate instrumentation filters for code coverage. This simplifies combining multiple shorthand configuration flags in bazelrc files. The existing --instrumentation_filter option is overridden when the new flag is present. The changes include modifications to BuildConfigurationValue.java and CoreOptions.java to implement the new option and its behavior, and the addition of new tests in StarlarkRuleContextTest.java to verify the functionality.
Highlights
- New CLI Option: Introduces
--experimental_instrumentation_filter_fragmentto accumulate instrumentation filters, addressing issue Accumulate multiple uses of --instrumentation_filter cli flag bazelbuild/bazel#22959. - Configuration Changes: Modifies
BuildConfigurationValue.javato handle the new option and prioritize it over--instrumentation_filter. - Option Definition: Adds the definition of the new option in
CoreOptions.java. - Testing: Adds new tests to
StarlarkRuleContextTest.javato validate the behavior of the new option.
Changelog
Click here to see the changelog
- src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java
- Adds import for
com.google.common.base.Joinerat line 18. - Adds import for
com.google.devtools.common.options.OptionsParsingExceptionat line 48. - Adds a private
instrumentationFilterfield of typeRegexFilterat line 148. - Initializes
instrumentationFilterbased on--experimental_instrumentation_filter_fragmentor--instrumentation_filterat lines 306-317. - Returns the value of the
instrumentationFilterfield ingetInstrumentationFilter()at line 586.
- Adds import for
- src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java
- Adds import for
com.google.common.base.Joinerat line 20. - Adds the
--experimental_instrumentation_filter_fragmentoption, which allows multiple values, at lines 279-292.
- Adds import for
- src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java
- Adds tests for
--experimental_instrumentation_filter_fragmentto verify its behavior with coverage instrumentation at lines 3024-3094.
- Adds tests for
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
A new flag appears,
Filters fragment, quell our fears,
Coverage now refined.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a new command-line option to accumulate instrumentation filters, which simplifies combining multiple shorthand configuration flags. The code is well-structured and includes tests to verify the new functionality. However, there are a few areas where the code can be improved, such as exception handling and test coverage.
Summary of Findings
- Instrumentation Filter Logic: The pull request introduces a new flag,
--experimental_instrumentation_filter_fragment, to accumulate instrumentation filters. The logic correctly prioritizes this new flag over the existing--instrumentation_filter. However, the exception handling inBuildConfigurationValue.javacould be improved to provide more context to the user. - Test Coverage: The pull request includes new tests to verify the behavior of the new flag. The tests cover cases where the flag is used multiple times, and when it's used in conjunction with the old flag. However, consider adding tests for edge cases, such as empty filter fragments or invalid regex patterns.
Merge Readiness
The pull request is almost ready for merging. Addressing the medium severity issue related to exception handling in BuildConfigurationValue.java would improve the user experience. While the tests cover the main functionality, adding tests for edge cases would increase the robustness of the code. Once these issues are addressed, the pull request should be ready for merging. I am unable to approve this pull request, and recommend that others review and approve this code before merging.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java(5 hunks)src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java(2 hunks)src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java(1 hunks)
🔇 Additional comments (12)
src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java (1)
278-292: Good implementation of the new experimental optionThis new option effectively adds the requested functionality to specify multiple instrumentation filter fragments that accumulate. The option is properly documented, with the accumulation behavior and precedence over the legacy option clearly explained in the help text.
A few observations:
- The option correctly uses
allowMultiple = trueto enable accumulation- The precedence is set so that this option overrides the legacy
--instrumentation_filterwhen specified- The help text is comprehensive and includes all necessary details
src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java (5)
148-148: LGTM: Clean field declarationThe field declaration for the new RegexFilter instance is appropriate and correctly marked as final.
586-586: LGTM: Getter returns the correct fieldThe getter now returns the newly computed field rather than directly accessing options.instrumentationFilter, which correctly implements the precedence described in the option's help text.
17-19: LGTM: Required import for JoinerThe import for Joiner is needed to support joining the filter fragments with commas.
48-48: LGTM: Required import for exception handlingThe import for OptionsParsingException is needed to handle potential parsing errors when converting the joined fragments to a RegexFilter.
306-317: Verify compatibility with the PR requirementsThe current implementation makes the experimental option override the legacy option when both are specified. However, in the PR objectives, the author mentioned a preference for the legacy option to override the experimental one.
The help text and implementation both make the experimental option take precedence, but the PR description mentioned a preference for the opposite behavior. Please confirm that the current precedence is the intended behavior.
src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java (6)
3023-3033: Well-written test for basic experimental flag behavior with coverage disabled.The test correctly verifies that when code coverage collection is disabled (
--nocollect_code_coverage), thecoverage_instrumented()method should return false, regardless of the experimental filter fragment setting. This establishes a good baseline for the experimental feature.
3036-3045: Good test for source file label behavior with experimental filter fragment.This test correctly verifies that source files are not instrumented, even when the experimental filter fragment is set. This follows the same behavior as the existing instrumentation filter and maintains consistency in the coverage system.
3048-3057: Proper test for filter non-matching behavior.This test correctly validates that a rule is not instrumented when it doesn't match the filter pattern specified by the experimental instrumentation filter fragment. The test demonstrates how negation patterns work with the new flag (
--experimental_instrumentation_filter_fragment=-:bar).
3060-3069: Good test for filter matching behavior.This test properly verifies that rules matching the filter pattern are correctly instrumented. It also demonstrates that multiple experimental filter fragments can be combined, which is the core functionality of this new feature.
3072-3083: Important test for precedence behavior.This test correctly validates that when both
--experimental_instrumentation_filter_fragmentand--instrumentation_filterare specified, the experimental filter takes precedence. This is a critical part of the feature as mentioned in the PR objectives.
3086-3095: Thorough test for non-default label matching.This test properly verifies that dependencies (non-default labels) are correctly instrumented when they match the experimental filter fragment. It completes the test suite with comprehensive coverage of the feature's behavior.
| if (!options.instrumentationFilterFragment.isEmpty()) { | ||
| // A regex-based instrumentation filter instance is formed by concatenating | ||
| // string values of --experimental_instrumentation_filter_fragment. | ||
| try { | ||
| this.instrumentationFilter = new RegexFilter.RegexFilterConverter().convert( | ||
| Joiner.on(",").join(options.instrumentationFilterFragment)); | ||
| } catch (OptionsParsingException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| } else { | ||
| this.instrumentationFilter = options.instrumentationFilter; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implementation handles the new option properly but could improve error handling
The implementation correctly joins multiple filter fragments and creates a RegexFilter when the experimental option is specified, falling back to the legacy filter otherwise.
Consider improving the error handling by including a more descriptive error message when wrapping the OptionsParsingException:
try {
this.instrumentationFilter = new RegexFilter.RegexFilterConverter().convert(
Joiner.on(",").join(options.instrumentationFilterFragment));
} catch (OptionsParsingException e) {
- throw new RuntimeException(e);
+ throw new RuntimeException("Failed to parse experimental_instrumentation_filter_fragment: " + e.getMessage(), e);
}This would make debugging easier by providing context about which option caused the error.
This option is similar to
--instrumentation_filteroption. The difference is this option can be used multiple times to accumulate its values. If this option is provided--instrumentation_filterhas no effect.Multiple uses of
--instrumentation_filterare not accumulated. This makes it difficult to have short hand configuration flags for a combination of coverage flags in a bazelrc file. e.g.,With a large set of flags and their respective large filter strings it is uneasy to combine the short hand configuration flags because the respective filter strings need to be concatenated into a single string. This is uneasy to maintain because filter strings are repeated in the combinations.
--experimental_instrumentation_filter_fragmentsimplifies combining multiple shorthand configuration flags because its multiple uses are accumulated. e.g.,Add tests for --experimental_instrumentation_filter_fragment cli to verify that its multiple uses are accumulated and --instrumentation_filter has no effect when --experimental_instrumentation_filter_fragment is used.
bazelbuild#22959 is the upstream issue.
Testing Done:
Summary by CodeRabbit