SED-4758 Implement Label Value Cardinality Safeguards for Custom Metrics Ingestion#660
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a per-execution label cardinality safeguard to prevent high cardinality on user-defined metric and measurement labels, along with a new execution notice system to raise and resolve lightweight, non-blocking warnings. Key changes include updating TimeSeriesMetricSamplesHandler to enforce unique-value quotas, introducing ExecutionNoticeManager and related classes to manage notices, and synchronizing ExecutionManager.updateExecution to handle concurrent updates safely. The review feedback suggests improving test assertions in ExecutionNoticeManagerTest and ExecutionNoticeResolverTest by using specific assertEquals checks instead of assertTrue with contains for deterministic outcomes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| * is raised once per label.</li> | ||
| * </ul> | ||
| */ | ||
| private String maskIfOverQuota(ExecutionContext executionContext, String execId, String metricName, |
There was a problem hiding this comment.
It took me time to understand that value means labelValue and not the value of the metric itself
| if (values.contains(value)) { | ||
| return value; | ||
| } | ||
| if (values.size() < maxUniqueLabelValues) { |
There was a problem hiding this comment.
Here we do the check to ensure that the number of label values per label remains under the defined max. Don't we want to also ensure that we don't exceed the number of labels?
There was a problem hiding this comment.
added now also a quota on the number of labels per exec id and measurement/metric name
| } | ||
| Map<String, String> params = (parameters != null) ? parameters : Collections.emptyMap(); | ||
| Matcher matcher = PLACEHOLDER.matcher(template); | ||
| StringBuffer result = new StringBuffer(); |
There was a problem hiding this comment.
I learnt many years ago that StringBuilder is more efficient. Not sure if still the case in 2026 and if this method is performance sensible
There was a problem hiding this comment.
You're right, this should be a StringBuilder
| */ | ||
| public class ResolvedExecutionNotice { | ||
|
|
||
| private String typeId; |
There was a problem hiding this comment.
Could use public fields instead
There was a problem hiding this comment.
Using public mutable fields breaks encapsulation, making code fragile because any external class can modify the DTO's data at runtime. If the goal was to achieve immutability (the primary reason someone would use a constructor-based @JsonCreator), making the fields public completely defeated that purpose. Now with target Java 21 we can use "record" instead.
| */ | ||
| public class ExecutionNotice { | ||
|
|
||
| private String typeId; |
There was a problem hiding this comment.
Could use public fields instead of getters / setters or maybe records?
There was a problem hiding this comment.
Actually using public field is an anti pattern and should not be used here. Luckily now that we moved to target Java 21 we can use the recommended approach for immutable Pojo with (de)serialization support: records, I made the change.
2 participants
No description provided.