-
Notifications
You must be signed in to change notification settings - Fork 40
Nocode class and method selectors #2304
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
Conversation
bootstrap/src/main/java/com/splunk/opentelemetry/javaagent/bootstrap/nocode/NocodeRules.java
Outdated
Show resolved
Hide resolved
...ntation/nocode/src/main/java/com/splunk/opentelemetry/instrumentation/nocode/YamlParser.java
Outdated
Show resolved
Hide resolved
...ntation/nocode/src/main/java/com/splunk/opentelemetry/instrumentation/nocode/YamlParser.java
Outdated
Show resolved
Hide resolved
...ntation/nocode/src/main/java/com/splunk/opentelemetry/instrumentation/nocode/YamlParser.java
Outdated
Show resolved
Hide resolved
And one more question. YAML files for declarative config use snake case naming convention, rather than camelCase. Maybe we should use the same? |
Good call. I updated all the old stuff to |
...ntation/nocode/src/main/java/com/splunk/opentelemetry/instrumentation/nocode/YamlParser.java
Outdated
Show resolved
Hide resolved
...ntation/nocode/src/main/java/com/splunk/opentelemetry/instrumentation/nocode/YamlParser.java
Outdated
Show resolved
Hide resolved
...ntation/nocode/src/main/java/com/splunk/opentelemetry/instrumentation/nocode/YamlParser.java
Outdated
Show resolved
Hide resolved
...de-testing/src/test/java/com/splunk/opentelemetry/instrumentation/nocode/YamlParserTest.java
Show resolved
Hide resolved
.../nocode/src/main/java/com/splunk/opentelemetry/instrumentation/nocode/NocodeInitializer.java
Outdated
Show resolved
Hide resolved
…instrumentation/nocode/NocodeInitializer.java Co-authored-by: Lauri Tulmin <[email protected]>
|
||
// formatting of the strings with newlines makes it easier to read and | ||
// reason about the test. | ||
// spotless:off |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
an alternative that could be considered would be to use text blocks since the tests run with java 17
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll see what's available when I reformat this whole thing for -contrib
.
} | ||
}; | ||
|
||
abstract <E extends NamedElement> ElementMatcher<E> parse( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll change the return type to ElementMatcher<? extends NamedElement>
. This will push the unchecked cast to the caller of the parse method in matcherFromKeyAndYamlValue
.
Add logic for more complex class/method selectors than just exact-name-matching:
I tried to keep the list/complexity as small as possible while still accounting for 95%+ of real-world usage. I had to add a separate
-testing
module to work around a classloader issue in order to properly test all the invalid parse logic.A larger classloader problem I encountered was that the yaml is clearly specifying bytebuddy
ElementMatcher
s (and the logic for producing and using them is pretty clean since the feature set offered by bytebuddy is so complete), but bytebuddy classes are not visible to the bootstrap-locatedRule
class.Rather than wrap theLauri suggested a fix separating a bootstrap interface forElementMatcher
logic in a bootstrap-only shim class, for now I'm exposing them as simpleObject
s (with some comments) since they are never used or invoked at the bootstrap layer. It's not a great design, and I'm happy to code up anything better that anybody else comes up with.Rule
from aRuleImpl
in agent space which can safely reference theElementMatcher
directly. Thanks, Lauri!