gianm commented on code in PR #18803:
URL: https://github.com/apache/druid/pull/18803#discussion_r2583678018
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/TaskRealtimeMetricsMonitorBuilder.java:
##########
@@ -41,13 +44,26 @@ public static TaskRealtimeMetricsMonitor build(
return new TaskRealtimeMetricsMonitor(
metrics,
meters,
- ImmutableMap.of(
- DruidMetrics.DATASOURCE, new String[]{task.getDataSource()},
- DruidMetrics.TASK_ID, new String[]{task.getId()},
- DruidMetrics.TASK_TYPE, new String[]{task.getType()},
- DruidMetrics.GROUP_ID, new String[]{task.getGroupId()}
- ),
+ getMetricDimensions(task),
Review Comment:
The `Map<String, String[]> dimensions` on `TaskRealtimeMetricsMonitor` could
be changed to `ServiceMetricEvent.Builder`, then it could be passed
`task.getMetricBuilder()` and avoid the duplicated logic / instanceof check
below.
##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/SeekableStreamIndexTask.java:
##########
@@ -197,6 +199,16 @@ public String getCurrentRunnerStatus()
return (status != null) ? status.toString() : null;
}
+ @Override
+ protected ServiceMetricEvent.Builder getMetricBuilder()
Review Comment:
Something is off here. `super.getMetricBuilder()` just returns
`metricBuilder`, so what's happening in this method is redundant. It would be
equivalent to do this in the constructor rather than `getMetricBuilder`. i.e.
simply do `metricBuilder.setDimension(DruidMetrics.SUPERVISOR_ID,
supervisorId)`.
However, this highlights a problem: the `metricBuilder` is not thread safe,
yet it appears to be used from multiple threads with the pattern
`emitter.emit(getMetricBuilder().setMetric(metric, value))`. For example, as
far as I can tell, `ingest/segments/count` is emitted in a push thread but
certain other metrics are emitted in the main thread. It probably is called
infrequently enough that this doesn't cause problems in practice. But it's
still a thread safety bug and we should fix it.
How about redefining this method to return a new instance of
`ServiceMetricEvent.Builder` each time it's called? That should fix it, and
then this implementation you have here would be correct.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
