Make ServiceMetricEvent immutable, do not allow null dimension values#18272
Make ServiceMetricEvent immutable, do not allow null dimension values#18272kfaraz merged 6 commits intoapache:masterfrom
Conversation
kfaraz
changed the title
| // This change is done to keep the code coverage tool happy by exercising the implementation | ||
| queryMetrics.sqlQueryId("dummy"); | ||
| queryMetrics.queryId("dummy"); | ||
| queryMetrics.reportQueryTime(0).emit(serviceEmitter); |
There was a problem hiding this comment.
Why was this change needed? 🤔
There was a problem hiding this comment.
Need to emit the event after all the dimensions are set. Otherwise, this test would fail.
There was a problem hiding this comment.
public Builder setDimension(String dim, Object value) and public Builder setDimensionIfNotNull(String dim, Object value) could probably be collapsed into a single method I think? Any reason for keeping them separate? is it to avoid widespread changes across the codebase?
There was a problem hiding this comment.
Yes, it keeps the patch small.
Also, the setDimensionIfNotNull is essentially a short hand which does the null check for you, while the other throws exception on nulls.
| */ | ||
| public Builder setDimensionIfNotNull(String dim, Object value) | ||
| { | ||
| if (dim == null) { |
There was a problem hiding this comment.
nit: as the name setDimensionIfNotNull suggests, consider only keep value check in this function, move the dim check and userDims.put logic to setDimension. Also consider use StringUtils.isEmpty to check dim is not empty string as well.
| this.createdTime = createdTime != null ? createdTime : DateTimes.nowUtc(); | ||
| this.serviceDims = serviceDims; | ||
| this.userDims = userDims; | ||
| this.userDims = userDims == null ? Map.of() : Map.copyOf(userDims); |
There was a problem hiding this comment.
nit: userDims could also be ImmutableMap in line 50.
There was a problem hiding this comment.
Yeah, that would make for better symmetry with the other field serviceDims, let me do that.
|
I think we can merge this PR since the IT failure is due to some issue with the k8s cluster setup for |
|
Thanks a lot for the reviews, @Akshat-Jain , @cecemei ! |
…apache#18272) Changes: - Make `ServiceMetricEvent` immutable. All fields were already immutable except `userDims`. - Do not allow null dimension names or values. - Remove `ServiceMetricEvent.copy` as the event is already immutable.
3 participants
Follow up to #18255 to handle null dimension values in
ServiceMetricEventRelated comment: #17170 (comment)
ServiceMetricEventshould ideally be immutable.Otherwise, there can be a race if the
ServiceMetricEvent.Builderis reused while the first eventis still being emitted.
Changes
ServiceMetricEventimmutable. All fields were already immutable. Fix upuserDimsto be immutable too.ServiceMetricEvent.copyas the event is already immutable.This PR has: