-
Notifications
You must be signed in to change notification settings - Fork 1k
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
fix: NPE when udf metrics enabled #5960
fix: NPE when udf metrics enabled #5960
Conversation
fixes: confluentinc#5890 Fixes an NPE thrown when the second instance of a UDAF is initialized. The NPE was being thrown due to a bug in the code that failed to initialise the sensor fields in the `UdafAggregateFunction` class if the sensor already existed. The commit also refactors the code used to add function invocation metrics such that all three function types: UDF, UDTF and UDAF, use the same function to register metrics. Unit tests added to ensure this single code path doesn't throw an NPE or return null on the subsequent invocation. Also added `ksql.udf.collect.metrics` metric to the server config file, so users are more likely to be able to find it and try it out. Moved UDTF functions out of the `ksql-udf` group and into `ksql-udtf` group. Having them in the former was another bug.
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.
LGTM, with two points of consideration
final Sensor existing = metrics.getSensor(sensorName); | ||
if (existing != null) { | ||
return existing; | ||
} | ||
|
||
final Sensor newSensor = metrics.sensor(sensorName); |
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.
there's a race here that is possible here where two calls for the same metrics.getSensor(sensorName)
don't find the sensor and they both end up registering the extra metrics. I don't know of a good way around it...
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.
Yeah I know, but I was just fixing the NPE and being lazy ;). Fixed it.
@@ -67,7 +68,8 @@ public void loadUdtfFromClass( | |||
} | |||
final String functionName = udtfDescriptionAnnotation.name(); | |||
final String sensorName = "ksql-udtf-" + functionName; | |||
FunctionLoaderUtils.addSensor(sensorName, functionName, metrics); | |||
|
|||
FunctionMetrics.initInvocationSensor(metrics, sensorName, "ksql-udtf", functionName + " udtf"); |
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 anyone was relying on the old metrics, their alerts for udtfs will break with this change. probably ok as I'd be shocked if anyone was actually writing their own UDTFs and using metrics on them at this point but something to call out 😉
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.
Yeah, I'm aware, but ... meh. turning metrics on caused an NPE, so likely no one is using them, and better to change it now.
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.
As mentioned in the description, I see this as a bug fix.
* fix: NPE when udf metrics enabled fixes: confluentinc#5890 Fixes an NPE thrown when the second instance of a UDAF is initialized. The NPE was being thrown due to a bug in the code that failed to initialise the sensor fields in the `UdafAggregateFunction` class if the sensor already existed. The commit also refactors the code used to add function invocation metrics such that all three function types: UDF, UDTF and UDAF, use the same function to register metrics. Unit tests added to ensure this single code path doesn't throw an NPE or return null on the subsequent invocation. Also added `ksql.udf.collect.metrics` metric to the server config file, so users are more likely to be able to find it and try it out. Moved UDTF functions out of the `ksql-udf` group and into `ksql-udtf` group. Having them in the former was another bug. Co-authored-by: Andy Coates <[email protected]>
Description
fixes: #5890
Fixes an NPE thrown when the second instance of a UDAF is initialized. The NPE was being thrown due to a bug in the code that failed to initialise the sensor fields in the
UdafAggregateFunction
class if the sensor already existed.The commit also refactors the code used to add function invocation metrics such that all three function types: UDF, UDTF and UDAF, use the same function to register metrics.
Unit tests added to ensure this single code path doesn't throw an NPE or return null on the subsequent invocation.
Also added
ksql.udf.collect.metrics
metric to the server config file, so users are more likely to be able to find it and try it out.Moved UDTF functions out of the
ksql-udf
group and intoksql-udtf
group. Having them in the former was another bug.Testing done
Added unit tests plus manual validation that metric names didn't change with new code, (except intentional move of UDTF metrics into own group).
Reviewer checklist