[server] Fix NPE when key value size profiling is not enabled #1378
Conversation
| requestKeySizeSensor.record(keySize); | ||
| if (requestKeySizeSensor != null) { | ||
| requestKeySizeSensor.record(keySize); | ||
| } |
There was a problem hiding this comment.
Hmm... but it's weird though... why is this sensor null?
The server config should control both whether this sensor is null, and also whether this function gets called or not 🤔
There was a problem hiding this comment.
What I mean is:
This function is only called from MultiGetResponseStatsWithSizeProfiling (and the equivalent compute-related class). And MultiGetResponseStatsWithSizeProfiling can only get constructed when serverConfig.isKeyValueProfilingEnabled() is true...
And AggServerHttpRequestStats also gets passed the same config, which (after a few indirections) should determine whether that sensor gets created or not.
So how can these be mismatched?
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
It's coming from the following stack trace.
java.lang.NullPointerException: Cannot invoke "io.tehuti.metrics.Sensor.record(double)" because "this.requestValueSizeSensor" is null
at com.linkedin.venice.stats.ServerHttpRequestStats.recordValueSizeInByte(ServerHttpRequestStats.java:452) ~[venice-server.jar:?]
at com.linkedin.venice.listener.response.stats.ResponseStatsUtil.consumeIntIfAbove(ResponseStatsUtil.java:22) ~[venice-server.jar:?]
at com.linkedin.venice.listener.response.stats.SingleGetResponseStats.recordMetrics(SingleGetResponseStats.java:30) ~[venice-server.jar:?]
at com.linkedin.venice.listener.ServerStatsContext.recordBasicMetrics(
In SingleGetResponseStats we've this method:
@Override
public void recordMetrics(ServerHttpRequestStats stats) {
super.recordMetrics(stats);
ResponseStatsUtil.consumeIntIfAbove(stats::recordValueSizeInByte, this.valueSize, 0);
stats.recordKeySizeInByte(this.keySize);
}
IMO in the above NPE case SingleGetResponseStats::valueSize is set from one of these places where we don't check size profiling config is enabled or not:
There was a problem hiding this comment.
This is regression due to: #1377. It assumed that single get request stats has similar logic to multi-key requests
There was a problem hiding this comment.
Sorry about that. Any idea why this wasn't caught in any of the tests?
There was a problem hiding this comment.
Oh yeah, right. I see it now: #1377 (comment)
There was a problem hiding this comment.
What's the motivation to disable KV size profiling for single gets?
There was a problem hiding this comment.
Metric bloat maybe? IDK. @nisargthakkar ?
There was a problem hiding this comment.
Any idea why this wasn't caught in any of the tests?
Probably we don't have E2E a test which verifies all metrics for single-get request type?
|
Closing this PR since @nisargthakkar updating KV size profiling behavior in #1381 |
Fix NPE when key value size profiling is not enabled
This commit addresses a NullPointerException in
ServerHttpRequestStatswhen attempting to record key or value sizes while the corresponding profiling sensors (requestKeySizeSensororrequestValueSizeSensor) are null.How was this PR tested?
UT
Does this PR introduce any user-facing changes?