You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "abhishekrb19 (via GitHub)" <gi...@apache.org> on 2026/04/18 00:17:34 UTC
[PR] minor: add getServiceDims() getter to ServiceMetricEvent (druid)
abhishekrb19 opened a new pull request, #19350:
URL: https://github.com/apache/druid/pull/19350
## Summary
Adds a `getServiceDims()` getter method to `ServiceMetricEvent` to provide direct access to the immutable service dimensions map. This can be used by custom extensions along with the existing getters in the class.
## Motivation
Previously, callers could access individual service dimension values via `getService()` and `getHost()`, but had no way to retrieve the complete service dimensions map. This getter provides direct access to the immutable map of all service
dimensions by custom extensions.
This PR has:
- [x] been self-reviewed.
- [x] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
--
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
Re: [PR] minor: add getServiceDims() getter to ServiceMetricEvent (druid)
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 commented on code in PR #19350:
URL: https://github.com/apache/druid/pull/19350#discussion_r3104150018
##########
processing/src/test/java/org/apache/druid/java/util/emitter/service/ServiceMetricEventTest.java:
##########
@@ -76,6 +76,9 @@ public void testBuilder()
builderEvent.toMap()
);
+ Assert.assertEquals("test", builderEvent.getServiceDims().get("service"));
Review Comment:
“service” and “host” are [documented](https://druid.apache.org/docs/latest/operations/metrics/) metric dimensions. Hence, they cannot change due to backward compatibility guarantees, and this test and other existing ones that assert on the literals would catch that unlikely drift.
--
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
Re: [PR] minor: add getServiceDims() getter to ServiceMetricEvent (druid)
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 merged PR #19350:
URL: https://github.com/apache/druid/pull/19350
--
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
Re: [PR] minor: add getServiceDims() getter to ServiceMetricEvent (druid)
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 commented on code in PR #19350:
URL: https://github.com/apache/druid/pull/19350#discussion_r3104141705
##########
processing/src/main/java/org/apache/druid/java/util/emitter/service/ServiceMetricEvent.java:
##########
@@ -93,6 +93,11 @@ public String getHost()
return serviceDims.get(HOST);
}
+ public ImmutableMap<String, String> getServiceDims()
Review Comment:
Yes, that's a good point - thanks for calling that out!
--
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
Re: [PR] minor: add getServiceDims() getter to ServiceMetricEvent (druid)
Posted by "utafrali (via GitHub)" <gi...@apache.org>.
utafrali commented on code in PR #19350:
URL: https://github.com/apache/druid/pull/19350#discussion_r3104072024
##########
processing/src/main/java/org/apache/druid/java/util/emitter/service/ServiceMetricEvent.java:
##########
@@ -93,6 +93,11 @@ public String getHost()
return serviceDims.get(HOST);
}
+ public ImmutableMap<String, String> getServiceDims()
Review Comment:
The return type `ImmutableMap<String, String>` leaks the concrete implementation, which is inconsistent with `getUserDims()` in the same class — that method returns `Map<String, Object>` even though its backing field is also an `ImmutableMap`. For a public API, prefer returning the interface type:
```java
public Map<String, String> getServiceDims()
{
return serviceDims;
}
```
Callers who need the immutability guarantee can check `instanceof ImmutableMap` or the Javadoc can document it. Returning the concrete type here makes it harder to swap the implementation later.
##########
processing/src/test/java/org/apache/druid/java/util/emitter/service/ServiceMetricEventTest.java:
##########
@@ -76,6 +76,9 @@ public void testBuilder()
builderEvent.toMap()
);
+ Assert.assertEquals("test", builderEvent.getServiceDims().get("service"));
Review Comment:
The string literals `"service"` and `"host"` duplicate the key constants already defined in the class (used inside `getService()` and `getHost()`). If those constants (`SERVICE`, `HOST`) are accessible from the test, use them directly to avoid silent drift if the keys ever change:
```java
Assert.assertEquals("test", builderEvent.getServiceDims().get(ServiceMetricEvent.SERVICE));
Assert.assertEquals("localhost", builderEvent.getServiceDims().get(ServiceMetricEvent.HOST));
```
If the constants are private, at minimum add a comment explaining why literals are used here.
--
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