-
Notifications
You must be signed in to change notification settings - Fork 166
Common Expression Language (CEL) sampler #1957
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
base: main
Are you sure you want to change the base?
Conversation
|
@dol this looks nice! @johnbley what do you think of CEL vs JEXL in the context of open-telemetry/opentelemetry-java-instrumentation#13590? |
Just a note to myself. I need to expand the test cases with the following addition.
|
Very interesting! I'll take a look at it! |
As I refined the tests I found some edge cases in the setup and mapping of the CEL engine. I'll mark the PR as work in progress. Sorry for that. |
no worries! you can click "Convert to draft" (hidden under the list of reviewers), and then when you're ready you can click "Ready for review" |
@trask @jack-berg : I wanted you to give an update on the PR. As I was adding more tests cases to PR I encountered a problem with single/double quote parsing on the declarative config. First I though it's an issue with a complex expression input. But after digging deep into the source code of this project and I created the following bug report: open-telemetry/opentelemetry-java#7429 As the chances are very high that a complex expression will need to mix single and double quotes, this bug should be addressed first. |
open-telemetry/opentelemetry-java#7433 should fix this regression. |
The latest version added more unit tests and better coverage. I think the new CEL sampler is ready for an review. |
Build is pretty broken. Can you look at this @dol thanks! |
@breedx-splk I'm waiting for the |
Adds new CelBasedSampler to the OpenTelemetry Java SDK, enabling advanced sampling rules using the Common Expression Language (CEL). It also includes updates to the existing RuleBasedRoutingSampler for consistency and clarity.
@jack-berg @trask The PR is ready for review after the fix was merged and release ( open-telemetry/opentelemetry-java#7433 ) and the BOM version was bumped ( 850933f#diff-df7d51fc1db73056c56958a9784e26310dae8ec239fb940820f5ddea4b655693L5 ) |
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.
Thanks! I think this is an awesome addition that users will start trying immediately. I left a few comments/questions that I'd like to see addressed...but this is looking solid.
fallback_sampler: | ||
always_on: |
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.
If this required or does it default to always_on
if not present? Might be nice to mention that here?
final CelAbstractSyntaxTree abstractSyntaxTree; | ||
final String expression; | ||
final Sampler delegate; |
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 believe our convention is to make these private and use explicit get
ters, or use @AutoValue
with a create()
method.
DeclarativeConfigProperties fallbackModel = config.getStructured("fallback_sampler"); | ||
if (fallbackModel == null) { | ||
throw new DeclarativeConfigException( | ||
"cel_based sampler .fallback_sampler is required but is null"); | ||
} | ||
Sampler fallbackSampler; | ||
try { | ||
fallbackSampler = DeclarativeConfiguration.createSampler(fallbackModel); | ||
} catch (DeclarativeConfigException e) { | ||
throw new DeclarativeConfigException( | ||
"cel_based sampler failed to create .fallback_sampler sampler", e); | ||
} |
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.
Factoring this out to a method called getFallbackModel()
could probably help to shrink this long method and improve readability. 👍🏻
# The fallback sampler to use if no expressions match. | ||
fallback_sampler: | ||
always_on: | ||
# List of CEL expressions to evaluate. Expressions are evaluated in order. |
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 think it would help to be very explicit. Calling out that it's in-order is great, but is it that the first expression to match is applied and the remaining do not get evaluated, or is it the last one to match "wins"?
DeclarativeConfiguration.parseAndCreate( | ||
new ByteArrayInputStream(yaml.getBytes(StandardCharsets.UTF_8))); | ||
Sampler sampler = openTelemetrySdk.getSdkTracerProvider().getSampler(); | ||
assertThat(sampler.toString()) |
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 think it's fine for this first pass (shouldn't gate this PR), but testing through the toString()
is a bit of an antipattern. We can look at ways of making that more testable later tho.
try { | ||
builder.recordAndSample(expression); | ||
} catch (CelValidationException e) { | ||
// Delegate to the provider to handle the exception |
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.
Same comment as above about fail/catch.
+ " - action: DROP\n" | ||
+ " expression: 'invalid cel expression!'\n", | ||
"Failed to compile CEL expression: invalid cel expression!")); | ||
} |
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.
Maybe I missed it, but it would be cool to add coverage for the case where expressions
exists but is empty.
CelBasedSampler.celCompiler | ||
.compile( | ||
"spanKind == 'SERVER' && attribute[\"" | ||
+ URL_FULL |
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.
This works because AttributeKey.toString()
merely returns the key (String), but if the toString()
one day had type information in it or something, this could fail. It can be made slightly more explicit/robust by using URL_FULL.getKey()
.
CelBasedSampler.celCompiler | ||
.compile( | ||
"spanKind == 'SERVER' && attribute[\"" | ||
+ URL_FULL |
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.
same comment as above
@ExtendWith(MockitoExtension.class) | ||
class CelBasedSamplerTest { | ||
private static final String SPAN_NAME = "MySpanName"; | ||
private static final SpanKind SPAN_KIND = SpanKind.SERVER; |
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.
In some of the tests at least, this additional redirection of an existing constant makes the tests slightly harder to read/understand. I'd just inline SpanKind.SERVER
instead of having this indirection.
This pull request introduces a new
CelBasedSampler
to the OpenTelemetry Java SDK, enabling advanced sampling rules using the Common Expression Language (CEL). It also includes updates to the existingRuleBasedRoutingSampler
for consistency and clarity. Below is a summary of the most important changes grouped by theme:New
CelBasedSampler
Implementation:CelBasedSampler
class, which evaluates CEL expressions to make sampling decisions based on span attributes. It includes a fallback sampler and supports multiple expressions. (samplers/src/main/java/io/opentelemetry/contrib/sampler/CelBasedSampler.java
)CelBasedSamplerBuilder
to constructCelBasedSampler
instances with methods for adding expressions and specifying actions (e.g.,DROP
orRECORD_AND_SAMPLE
). (samplers/src/main/java/io/opentelemetry/contrib/sampler/CelBasedSamplerBuilder.java
)CelBasedSamplingExpression
class to encapsulate individual CEL expressions and their associated samplers. (samplers/src/main/java/io/opentelemetry/contrib/sampler/CelBasedSamplingExpression.java
)CelBasedSampler
, enabling configuration through YAML files. (samplers/src/main/java/io/opentelemetry/contrib/sampler/internal/CelBasedSamplerComponentProvider.java
)Documentation Updates:
samplers/README.md
to document the newCelBasedSampler
, including its usage, schema, and example configurations. (samplers/README.md
) [1] [2]Dependency Additions:
dev.cel:cel:0.9.0
library to enable CEL expression evaluation. (samplers/build.gradle.kts
)Updates to
RuleBasedRoutingSampler
:SamplingRule
toRuleBasedRoutingSamplingRule
for clarity and updated all related references in theRuleBasedRoutingSampler
and its builder. (samplers/src/main/java/io/opentelemetry/contrib/sampler/RuleBasedRoutingSampler.java
,samplers/src/main/java/io/opentelemetry/contrib/sampler/RuleBasedRoutingSamplerBuilder.java
,samplers/src/main/java/io/opentelemetry/contrib/sampler/RuleBasedRoutingSamplingRule.java
) [1] [2] [3] [4] [5] [6] [7]These changes collectively enhance the SDK's sampling capabilities, allowing users to define sophisticated sampling rules using CEL while maintaining compatibility with existing samplers.