fix: support spaces in directive argument values#665
Open
rarguelloF wants to merge 8 commits intomainfrom
Open
fix: support spaces in directive argument values#665rarguelloF wants to merge 8 commits intomainfrom
rarguelloF wants to merge 8 commits intomainfrom
Conversation
Contributor
There was a problem hiding this comment.
Can you add a test that demonstrates what happens in cases where the quoted string is broken (i.e, never closed / no space after close & before next / ...)? Those seem to be missing and they're the edge cases I worry about :)
Contributor
Author
There was a problem hiding this comment.
Done! added a few more test cases, please let me know if the behavior makes sense, or if there's any other test missing 🙏
Comment on lines
+112
to
+147
| { | ||
| name: "unclosed double quote value", | ||
| prefix: "//dd:span", | ||
| comment: `//dd:span service.name:"my-service`, | ||
| want: []DirectiveArgument{ | ||
| {Key: "service.name", Value: `"my-service`}, | ||
| }, | ||
| wantOk: true, | ||
| }, | ||
| { | ||
| name: "unclosed single quote value", | ||
| prefix: "//dd:span", | ||
| comment: `//dd:span service.name:'my-service`, | ||
| want: []DirectiveArgument{ | ||
| {Key: "service.name", Value: "'my-service"}, | ||
| }, | ||
| wantOk: true, | ||
| }, | ||
| { | ||
| name: "missing space between quoted args", | ||
| prefix: "//dd:span", | ||
| comment: `//dd:span service.name:"my-service"resource.name:"GET /"`, | ||
| want: []DirectiveArgument{ | ||
| {Key: "service.name", Value: `my-service"resource.name:"GET /`}, | ||
| }, | ||
| wantOk: true, | ||
| }, | ||
| { | ||
| name: "quote starts but ends with different quote type", | ||
| prefix: "//dd:span", | ||
| comment: `//dd:span service.name:"my-service'`, | ||
| want: []DirectiveArgument{ | ||
| {Key: "service.name", Value: `"my-service'`}, | ||
| }, | ||
| wantOk: true, | ||
| }, |
Contributor
There was a problem hiding this comment.
I find those surprising -- I reckon it's almost guaranteed that the outcome is not what the user intended...
Contributor
Author
There was a problem hiding this comment.
which one concretely? what do you think it should be the preferred behavior?
Contributor
There was a problem hiding this comment.
I'd expect a lot of the "improperly terminated" cases to result in an error or a warning being emitted.
Member
There was a problem hiding this comment.
@rarguelloF Ensure that the value is a valid Go string literal using double quote, fail on anything else (unless it's an unquoted string without spaces).
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #665 +/- ##
==========================================
+ Coverage 65.72% 69.89% +4.17%
==========================================
Files 113 116 +3
Lines 7926 6929 -997
==========================================
- Hits 5209 4843 -366
+ Misses 2192 1541 -651
- Partials 525 545 +20
🚀 New features to boost your workflow:
|
6 tasks
y1yang0
added a commit
to open-telemetry/opentelemetry-go-compile-instrumentation
that referenced
this pull request
Mar 25, 2026
#375) ## Description Adds a new **Directive Rule** — a join point that matches AST nodes annotated with magic comments (e.g., `//dd:span span.name:"my operation"`). This is a **pure filter** with no advice: it matches during the setup phase but performs no AST transformation during instrumentation. It becomes useful when `all-of` combinators land, allowing directive annotations to be combined with other join points. ### What's included - **New rule type: `directive`** — matches source files containing a `//directive` comment. The directive name is specified without the `//` prefix. Validation rejects empty directives, directives with spaces, and directives that start with `//`. - **Directive matching** (`matchDirective`) — validates that the comment is a line comment (not block), has no space after `//`, matches the full directive name (not a prefix), and requires whitespace after the name if arguments follow. Ports the matching semantics from Orchestrion's `directive.matches`. - **Argument parsing** (`ParseDirectiveArgs`, `scanArgs`, `tokenize`) — parses `key:value` arguments from directive comments. Values may be Go double-quoted strings (handled via `strconv.Unquote`). Single quotes and malformed input are rejected. This infrastructure is ready for use when combinators wire directive args into the instrument phase. - **Setup integration** — `createRuleFromFields` dispatches on the `directive` YAML key. `preciseMatching` uses a lazy-loaded `os.ReadFile` + `bytes.Contains` pre-filter (since `ParseFileFast` skips comments, the raw bytes are needed). - **Instrument phase** — directive rules are intentionally **excluded from `groupRules`** to avoid a wasted parse+write cycle for files that only have directive rules (no AST transformation to apply). - **Tests** — unit tests for matching (10 cases), arg parsing (9 cases), directive args (3 cases), rule validation (5 cases), rule factory (2 cases), `TestGroupRules` exclusion, and a golden integration test. ### Rule format ```yaml my_directive_rule: target: main directive: "otelc:span" ``` ### Rule fields | Field | Type | Required | Description | |-------------|--------|----------|-------------| | `directive` | string | yes | Directive name to match (without `//`) | ### Design decisions - **Pure filter, no advice** — no `before`/`after`/`assign_value` fields. The rule only participates in setup-phase matching. - **`bytes.Contains` pre-filter** — a loose but fast check on raw file bytes. False positives (e.g., the directive appearing inside a string literal) are acceptable because the precise `matchDirective` function will be used for real matching when combinators land. - **Excluded from `groupRules`** — since `applyDirectiveRule` would be a no-op, directive rules don't enter the instrument phase's file-processing loop, avoiding unnecessary AST parse+write cycles. ## Motivation Implements the Orchestrion-equivalent `directive` join point for compile-time instrumentation. This is a prerequisite for the `all-of` combinator feature, which will allow rules like "instrument functions annotated with `//dd:span`" by combining directive matching with function join points. Design follows feedback on Orchestrion PR DataDog/orchestrion#665 regarding Go double-quoted string support for directive argument values. --- ## Checklist - [x] PR title follows [conventional commits](https://www.conventionalcommits.org/) format - [x] Code formatted: `make format` - [x] Linters pass: `make lint` - [x] Tests pass: `make test` - [x] Tests added for new functionality - [x] Documentation updated (if applicable) --------- Co-authored-by: Yi Yang <qingfeng.yy@alibaba-inc.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds support for spaces in directive argument values, either using single or double quotes, as in:
This is useful since its pretty common to use spaces in span or resource names, and in span tag values in general.