Re: [PR] Restore Sink Metric Emission Behaviour: Emit them per-Sink instead of per-FireHydrant (druid)

["kfaraz (via GitHub)" <gi...@apache.org>]

kfaraz commented on code in PR #17170:
URL: https://github.com/apache/druid/pull/17170#discussion_r2209227633


##########
processing/src/main/java/org/apache/druid/java/util/emitter/service/ServiceMetricEvent.java:
##########
@@ -197,7 +198,7 @@ public ServiceMetricEvent build(ImmutableMap<String, String> serviceDimensions)
       return new ServiceMetricEvent(
           createdTime,
           serviceDimensions,
-          userDims,
+          new HashMap<>(userDims),

Review Comment:
   I think we should have gone here with making an immutable copy of the map in the builder after all.
   Otherwise, there is a potential race condition.
   
   Since the event builder can be reused, it is possible that the user dims of an event are updated even before the event has been emitted (since emission is always async). This would be even more likely to happen when there are multiple emitters.
   
   The other alternative would be to disallow reuse of the event builder, but it does offer some convenience when emitting multiple related events.
   `ServiceMetricEvent` should be immutable anyway.
   
   cc: @gianm , @kgyrtkirk 



-- 
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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org