[FLINK-40293][table] Add UDF metric config options and instrument sync scalar and table UDF calls - #28879
[FLINK-40293][table] Add UDF metric config options and instrument sync scalar and table UDF calls#28879weiqingy wants to merge 2 commits into
Conversation
cf258d6 to
720f7b0
Compare
720f7b0 to
dfb4c7c
Compare
…c scalar and table UDF calls Introduce two opt-in configuration options for FLIP-485 UDF metrics: table.exec.udf-metric-enabled (default false) and table.exec.udf-metric.sample-interval (default 100). Wrap the generated eval call site for sync scalar and table user-defined functions (via the BridgingSqlFunction stack) with sampled udfProcessingTime timing and udfExceptionCount counting, using the UdfMetrics helper registered on the operator metric group under udf.<udfName>. Instrumentation is emitted only when table.exec.udf-metric-enabled is true; the generated operator is byte-identical when disabled. One shared handle is registered per (operator, udfName). Lookup-join, ML-predict, vector-search, legacy CallGens, and PROCESS_TABLE functions are not metered.
dfb4c7c to
04d04ff
Compare
|
@HuangZhenQiu @RocMarshal PR-1 is merged, thanks! This is PR-2 of the FLIP-485 series, now rebased onto master so the diff is just the config options and the sync instrumentation. Would you be able to take a look when you have time? |
There was a problem hiding this comment.
Pull request overview
This PR advances FLIP-485 in the Table/SQL stack by introducing configuration switches for per-operator UDF metrics and wiring synchronous scalar/table UDF call-sites to emit timing and exception metrics via code generation, while ensuring the feature is fully gated off by default.
Changes:
- Added two streaming
ExecutionConfigOptionsto enable UDF metrics and configure sampling interval, plus regenerated execution config docs. - Instrumented generated synchronous scalar and table UDF
evalcall sites with sampled processing-time measurement and exception counting, gated at codegen time. - Added an end-to-end ITCase validating metric naming/scope, enable/disable behavior, exception counting, and handle sharing for repeated call sites.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/runtime/stream/sql/UdfMetricsITCase.java | New ITCase covering sync UDF metric emission, naming, gating, exception counting, and handle reuse. |
| flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/CodeGeneratorContext.scala | Caches one reusable UdfMetrics handle per UDF name within a generated operator to avoid duplicate registration being dropped. |
| flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/calls/BridgingSqlFunctionCallGen.scala | Opts “true” user-UDF calls into metric naming by passing the UDF name into the shared call-gen utility. |
| flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/calls/BridgingFunctionGenUtil.scala | Adds codegen-time instrumentation wrapper around sync scalar/table UDF eval statements (sampled timing + exception counting). |
| flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/config/ExecutionConfigOptions.java | Introduces the new UDF metric config options as @PublicEvolving streaming table options. |
| docs/layouts/shortcodes/generated/execution_config_configuration.html | Regenerated config option documentation including the two new UDF metric options. |
Suppressed comments (1)
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/runtime/stream/sql/UdfMetricsITCase.java:83
- exceptionCountPattern has the same unquoted-regex construction issue as processingTimePattern; quoting the name avoids accidental regex interpretation.
private static String exceptionCountPattern(String udfName) {
return "\\.udf\\." + udfName + "\\.udfExceptionCount";
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // This is the async table function correlate entry; opt it into metrics under the UDF name, | ||
| // mirroring BridgingSqlFunctionCallGen for the other UDF kinds. | ||
| udfMetricName = Some(function.getName) |
There was a problem hiding this comment.
Good catch, that over-claimed. Reworded: the entry supplies the UDF name that instrumented call generators scope their metrics under. Instrumenting the async-table path is FLINK-40294, the next PR in the series.
| .withDescription( | ||
| "When UDF metrics are enabled, udfProcessingTime is measured every N " | ||
| + "invocations (default 100). The non-sampled fast path is a single integer increment."); |
There was a problem hiding this comment.
Fixed. The description now says the value must be at least 1, and that 1 measures every invocation, matching the check in UdfMetrics.register. Config docs regenerated.
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| } |
There was a problem hiding this comment.
Fixed, it now rethrows after restoring the interrupt flag.
Small correction: the original didn't swallow it. Thread.currentThread().interrupt() restores the flag, which is the usual idiom. Your underlying point was right though, skipping the sleep would have failed on a timing assertion instead of saying it was interrupted.
| long minExpectedNanos = TimeUnit.MILLISECONDS.toNanos(SLEEP_MILLIS); | ||
| assertThat(stats.getMax()).isGreaterThanOrEqualTo(minExpectedNanos); | ||
| assertThat(stats.getMean()).isGreaterThanOrEqualTo(minExpectedNanos); |
There was a problem hiding this comment.
Would it be OK to keep the current threshold? Thread.sleep overshoots rather than undershoots, since the OS rounds up to the next timer tick, so the only way to skip the sleep is an interrupt, which now throws.
A lower threshold would also stop the test showing that the histogram reflects a real delay, which is what it exists to prove.
| private static String processingTimePattern(String udfName) { | ||
| return "\\.udf\\." + udfName + "\\.udfProcessingTime"; | ||
| } |
There was a problem hiding this comment.
Every name reaching these helpers is a lowercase literal declared in this file (scalarudf, sleepyudf, tableudf), so is there a case that needs escaping today? The name-built patterns feed positive lookups that fail loudly if they miss, and the "nothing is registered" checks use the fixed ANY_* constants instead.
Happy to add Pattern.quote if you'd rather have the guard anyway.
…d tests Document that table.exec.udf-metric.sample-interval must be at least 1 and that a value of 1 measures every invocation, matching the check in UdfMetrics.register, and regenerate the config docs. Reword the async table function correlate comment so it no longer reads as if that path were already metered; it supplies the UDF name that instrumented call generators scope their metrics under. Fail fast when SleepyDoubler is interrupted instead of skipping the sleep, so an interruption surfaces as itself rather than as a confusing timing assertion failure.
|
@HuangZhenQiu @RocMarshal Copilot's review is addressed. Three of the five became a small fix commit: the sample-interval bounds are now in the option description, the async-table comment no longer reads as if that path were metered, and the sleep-based test fails fast on interrupt. The other two I answered in the threads rather than changed, and I'm happy to be overruled on either. Ready for review whenever you have time. |
This is the second PR of the FLIP-485 implementation, split into a stack of small, independently reviewable PRs under the umbrella issue FLINK-38071. Landing order:
UdfMetricshelper: registration, sampling, timing, exception counting (#28878)PR-1 (#28878) is merged, so this PR is rebased onto current master and the diff below is only this step's changes.
What is the purpose of the change
Adds the two configuration options and the first end-to-end slice of FLIP-485: synchronous scalar and table UDF calls instrumented at code generation, using the
UdfMetricshelper added in PR-1.Two metrics are registered on the executing operator's
OperatorMetricGroup, scoped as<operator_name>.udf.<udf_name>.<metric>:udfProcessingTime, a Histogram of per-invocation UDF time.udfExceptionCount, a Counter of exceptions that escape user code.The feature is off by default (
table.exec.udf-metric-enabled = false) with zero overhead when disabled: the instrumentation is emitted at code generation only when the option is on, so the generated operator is byte-identical to today when it is off. When on, only every Nth invocation is timed (table.exec.udf-metric.sample-interval, default 100), while exceptions are counted on every invocation.Brief change log
@PublicEvolvingoptions toExecutionConfigOptions:table.exec.udf-metric-enabled(default false) andtable.exec.udf-metric.sample-interval(default 100), plus the regenerated config docs.evalcall site for synchronous scalar and table UDFs inBridgingFunctionGenUtil, bracketing it with sampled timing and exception counting.UdfMetricshandle per(operator, udf name)inCodeGeneratorContext, so repeated call sites of the same function in one operator share a handle instead of the second registration being dropped.BridgingSqlFunctionstack, via an opt-in name passed fromBridgingSqlFunctionCallGen. Lookup-join, ML-predict, vector-search, legacyCallGens, andPROCESS_TABLEfunctions are not metered.Verifying this change
This change added tests and can be verified as follows:
UdfMetricsITCase(integration, usingInMemoryReporter) covers synchronous scalar and table functions, metric naming and scope, exception counting, the enabled/disabled gate, that a lookup join is not metered, that repeated call sites of one function share a single handle while distinct functions get separate ones, and thatudfProcessingTimereflects a real induced delay rather than only a sample count.ConfigOptionsDocsCompletenessITCaseand the regenerated config docs verify the two new options are documented.On overhead: the disabled path is byte-identical to today, so it is zero by construction. With the feature on, a local micro-benchmark put the non-sampled fast path in the sub-nanosecond range, since it is a single integer increment, with the
nanoTime()and histogram work amortized across the sampling interval. A rigorous JMH benchmark belongs inflink-benchmarksrather than here, and I am happy to follow up with one.Does this pull request potentially affect one of the following parts:
@Public(Evolving): yes, two new@PublicEvolvingConfigOptions inExecutionConfigOptions.Documentation
metrics.mdsection lands in PR-4.Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Anthropic Claude Opus 4.8 and Claude Opus 5)