Make Telemetry Tags Immutable

[sakrah]

Problem

The Tags class in libs/telemetry is the dimension container for every metric emission in OpenSearch. On high-throughput clusters, tag construction and lookup become a significant CPU overhead on the metrics hot path -- before any actual metric work (atomic increment, gauge read) occurs.

The root cause is structural:

• Mutable HashMap backend -- every Tags.create() allocates a new HashMap. Every metric call that passes tags pays this cost, even when the exact same tag combination fires millions of times per second.
• Unsafe as map keys -- because Tags is mutable and has no hashCode()/equals() overrides, metric caches require defensive copies on every lookup, and every consumer must build its own caching layer.
• No precomputed hash -- every map lookup involving Tags must iterate all entries to compute a hash.

This means the existing API structurally prevents zero-allocation metric hot paths. Caching-only approaches reduce overhead but cannot eliminate it -- the cache key itself is an allocation, and every new metric consumer rediscovers the same problem.

Solution

Replace the mutable HashMap-backed Tags with an immutable sorted-array implementation with a precomputed hash, inspired by Micrometer's Tags (sorted-array merge) and OpenTelemetry's ArrayBackedAttributes (parallel arrays, precomputed hash, last-wins dedup).

Internal storage

public final class Tags {
    private final String[] keys;      // sorted by key
    private final Object[] values;    // parallel to keys
    private final int hashCode;       // precomputed at construction -- O(1) return
}

New API

Method	Description
Tags.of(key, value)	1-tag factory, no sort needed
Tags.of(k1, v1, k2, v2)	2-tag factory, conditional swap
Tags.of(k1, v1, k2, v2, k3, v3)	3-tag factory
Tags.of(k1, v1, ..., k4, v4)	4-tag factory
Tags.of(String... keyValues)	N-pair varargs factory
Tags.concat(a, b)	Merge-sort two Tags; b wins on collision
Tags.fromMap(Map)	Bridge from existing map-based callers
size(), getKey(i), getValue(i)	Direct array access
equals(), hashCode()	Content-based, safe as map keys

Backward compatibility

Tags.create() and all addTag() overloads are deprecated, not removed. create() returns Tags.EMPTY. Each addTag() returns a new immutable instance via Tags.concat(this, Tags.of(key, value)). Existing fluent chains like Tags.create().addTag("a", "1").addTag("b", "2") compile and produce correct results -- they just allocate more than the equivalent Tags.of("a", "1", "b", "2").

Why this matters for downstream consumers

With immutable Tags and a precomputed hash, metric consumers can:

• Store Tags in fields and reuse them across calls without re-allocating
• Use Tags directly as map keys without defensive copies
• Share Tags across threads without synchronization
• Merge tag sets efficiently via concat() on pre-sorted arrays

Test plan

• 40 test cases in new TagsTests.java covering:
  • All factory methods (of overloads, fromMap, varargs)
  • Sorting invariants (keys always sorted regardless of input order)
  • Deduplication semantics (last value wins on duplicate keys)
  • concat() -- partial overlap, full overlap, interleaved, large remainder, hash consistency, null/empty inputs
  • equals()/hashCode() contract
  • getTagsMap() (String conversion, type preservation)
  • Backward compatibility (create(), addTag() chaining, all value types)
  • Map-key usability (content-based lookup across different instances)
• ./gradlew :libs:opensearch-telemetry:precommit passes

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

• [ x] Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 47bebbd)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Make Tags immutable with sorted-array backend and tests Relevant files: libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java libs/telemetry/src/test/java/org/opensearch/telemetry/metrics/tags/TagsTests.java Sub-PR theme: Fix callers to use new immutable Tags return values Relevant files: server/src/main/java/org/opensearch/index/autoforcemerge/AutoForceMergeMetrics.java test/telemetry/src/main/java/org/opensearch/test/telemetry/TestInMemoryHistogram.java

⚡ Recommended focus areas for review Hash Collision Risk The computeHash method starts with result = 1 and uses a standard polynomial hash. The EMPTY singleton is constructed with a hardcoded hashCode of 1, which matches the result of computeHash on empty arrays. However, this is fragile — if computeHash logic ever changes, the hardcoded value 1 in EMPTY will silently diverge. Consider computing the hash via computeHash(EMPTY_KEYS, EMPTY_VALUES) instead. public static final Tags EMPTY = new Tags(EMPTY_KEYS, EMPTY_VALUES, 1); Array Mutability The fromPairs method mutates the caller-provided rawKeys and rawValues arrays (insertion sort and dedup in-place). In ofStringPairs, these arrays are freshly allocated so mutation is safe. However, the Javadoc says "Mutates the provided arrays" which is a footgun if this private method is ever called with externally-owned arrays. Consider documenting this contract more strictly or making a defensive copy. private static Tags fromPairs(String[] rawKeys, Object[] rawValues, int count) { if (count == 0) return EMPTY; for (int i = 1; i < count; i++) { String key = rawKeys[i]; Object val = rawValues[i]; int j = i - 1; while (j >= 0 && rawKeys[j].compareTo(key) > 0) { rawKeys[j + 1] = rawKeys[j]; rawValues[j + 1] = rawValues[j]; j--; } rawKeys[j + 1] = key; rawValues[j + 1] = val; } int w = 0; for (int i = 0; i < count; i++) { if (w > 0 && rawKeys[w - 1].equals(rawKeys[i])) { rawValues[w - 1] = rawValues[i]; } else { rawKeys[w] = rawKeys[i]; rawValues[w] = rawValues[i]; w++; } } String[] keys = (w == count) ? rawKeys : Arrays.copyOf(rawKeys, w); Object[] values = (w == count) ? rawValues : Arrays.copyOf(rawValues, w); return new Tags(keys, values, computeHash(keys, values)); } Missing @Deprecated The create() factory method is kept for backward compatibility but is not annotated with @Deprecated. Since the PR description and tests label it as a "deprecated API", it should be annotated to signal to callers that they should migrate to of(...) or ofStringPairs(...). public static Tags create() { return EMPTY; } Key Semantics Change The histogramValueForTags map key was changed from HashMap<String, ?> to Map<String, ?>. The record method now calls tags.getTagsMap() which returns an unmodifiable Map.ofEntries(...) instance. Two calls with equal tags will produce equal map keys (since Map.ofEntries equality is entry-based), but this relies on the Map equality contract. This is correct but worth verifying that all consumers of getHistogramValueForTags() are aware the key type changed and that lookups still work as expected. Map<String, ?> tagsMap = tags.getTagsMap(); histogramValueForTags.put(tagsMap, value);

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 47bebbd

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Use consistent hash computation for EMPTY singleton The hardcoded hash code 1 for EMPTY is inconsistent with the computeHash method, which would return 1 for empty arrays (since result starts at 1 and the loop doesn't execute). While the result happens to be the same, it is fragile and misleading. Use computeHash(EMPTY_KEYS, EMPTY_VALUES) to keep the initialization consistent and self-documenting. libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java [31] -public static final Tags EMPTY = new Tags(EMPTY_KEYS, EMPTY_VALUES, 1); +public static final Tags EMPTY = new Tags(EMPTY_KEYS, EMPTY_VALUES, computeHash(EMPTY_KEYS, EMPTY_VALUES)); Suggestion importance[1-10]: 5 __ Why: The hardcoded 1 for EMPTY's hash is fragile if computeHash ever changes its initial value. Using computeHash(EMPTY_KEYS, EMPTY_VALUES) makes the code self-consistent and more maintainable, though it's a minor style/correctness concern.	Low
	Guard merge against unsorted input arrays The concat method assumes that both a and b have their keys sorted, but there is no enforcement of this invariant at construction time for all code paths. Specifically, the merge logic uses a merge-sort approach that only works correctly if both input arrays are already sorted. If an unsorted Tags instance were ever constructed (e.g., via a future code path), the merge would silently produce incorrect results. Consider adding an assertion or a sort step at the start of concat to guard against this. libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java [126-128] public static Tags concat(Tags a, Tags b) { if (a == null || a.keys.length == 0) return (b != null) ? b : EMPTY; if (b == null || b.keys.length == 0) return a; + assert isSorted(a.keys) : "Tags 'a' keys are not sorted"; + assert isSorted(b.keys) : "Tags 'b' keys are not sorted"; Suggestion importance[1-10]: 3 __ Why: The suggestion adds defensive assertions for sorted input in concat, but all construction paths in the class already enforce sorted order (via fromPairs, fromMap, or single-element factories). The risk is theoretical and the improvement is minor.	Low
Possible issue	Handle null tags argument in record method The record method does not handle a null tags argument, which could cause a NullPointerException. Additionally, getTagsMap() now returns an unmodifiable Map.of()-based map, which has value-equality semantics and works correctly as a ConcurrentHashMap key. However, if tags is null, the code will throw. Add a null check and fall back to recording without tags. test/telemetry/src/main/java/org/opensearch/test/telemetry/TestInMemoryHistogram.java [56-59] public synchronized void record(double value, Tags tags) { + if (tags == null || tags.size() == 0) { + histogramValue.addAndGet((int) value); + return; + } Map<String, ?> tagsMap = tags.getTagsMap(); histogramValueForTags.put(tagsMap, value); } Suggestion importance[1-10]: 3 __ Why: While the null-check is a valid defensive measure, the record(double, Tags) interface contract likely requires non-null tags, and the existing record(double) method handles the no-tags case. This is a test utility class, so the practical impact is low.	Low

---

Previous suggestions

Suggestions up to commit ed54255

Category	Suggestion	Impact
General	Always copy arrays to preserve immutability guarantee The fromPairs method mutates the caller-provided arrays in-place (sorting and deduplication). When w == count (no duplicates), the original rawKeys/rawValues arrays are stored directly in the Tags instance without copying. Since callers like of(k1,v1,k2,v2,k3,v3) pass freshly-allocated arrays this is safe today, but it is a fragile contract. If any future caller reuses those arrays, the immutability guarantee is broken. Always copy the arrays before storing them, or document the ownership transfer clearly. libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java [407-409] -private static Tags fromPairs(String[] rawKeys, Object[] rawValues, int count) { - if (count == 0) return EMPTY; +String[] keys = Arrays.copyOf(rawKeys, w); +Object[] values = Arrays.copyOf(rawValues, w); +return new Tags(keys, values, computeHash(keys, values)); - for (int i = 1; i < count; i++) { - String key = rawKeys[i]; - Object val = rawValues[i]; - int j = i - 1; - while (j >= 0 && rawKeys[j].compareTo(key) > 0) { - rawKeys[j + 1] = rawKeys[j]; - rawValues[j + 1] = rawValues[j]; - j--; - } - rawKeys[j + 1] = key; - rawValues[j + 1] = val; - } - ... - String[] keys = (w == count) ? rawKeys : Arrays.copyOf(rawKeys, w); - Object[] values = (w == count) ? rawValues : Arrays.copyOf(rawValues, w); - return new Tags(keys, values, computeHash(keys, values)); -} - Suggestion importance[1-10]: 4 __ Why: The suggestion is valid in principle - when w == count, the original rawKeys/rawValues arrays are stored directly. However, all current callers pass freshly-allocated arrays, so the risk is low. The improvement is a minor defensive coding practice rather than a critical bug fix.	Low
	Unify two-pair deduplication with shared helper When k1.equals(k2) (duplicate key), only v2 is kept, which is consistent with "last value wins". However, the validateValue calls for both v1 and v2 happen before this check, which is correct. The issue is that the deduplication logic here is inconsistent with fromPairs used by the 3- and 4-argument overloads: in fromPairs, the last occurrence wins during the deduplication pass (iterating forward), but in the 2-argument of, v2 wins because k2/v2 is used. While the result is the same here, the of(k1,v1,k2,v2) fast-path bypasses fromPairs entirely, so any future change to deduplication semantics in fromPairs won't be reflected here. Consider delegating to fromPairs for consistency. libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java [70-91] public static Tags of(String k1, Object v1, String k2, Object v2) { - ... - int cmp = k1.compareTo(k2); - String[] keys; - Object[] values; - if (cmp < 0) { - keys = new String[] { k1, k2 }; - values = new Object[] { v1, v2 }; - } else if (cmp > 0) { - keys = new String[] { k2, k1 }; - values = new Object[] { v2, v1 }; - } else { - keys = new String[] { k2 }; - values = new Object[] { v2 }; - } - return new Tags(keys, values, computeHash(keys, values)); + Objects.requireNonNull(k1, "k1 must not be null"); + Objects.requireNonNull(v1, "v1 must not be null"); + Objects.requireNonNull(k2, "k2 must not be null"); + Objects.requireNonNull(v2, "v2 must not be null"); + validateValue(v1); + validateValue(v2); + return fromPairs(new String[] { k1, k2 }, new Object[] { v1, v2 }, 2); } Suggestion importance[1-10]: 3 __ Why: The suggestion to delegate to fromPairs for consistency is a valid refactoring concern, but the current behavior is functionally correct and the deduplication semantics (v2 wins) are identical. This is a minor maintainability improvement with low impact.	Low
	Guard against null map keys in fromMap map.keySet().toArray(new String[0]) will throw an ArrayStoreException at runtime if the map contains non-String keys (e.g., a raw Map or Map<Object,?> passed via an unchecked cast). The method signature accepts Map<String, ?> so this is a compile-time contract, but a defensive check or using a typed stream would make the failure message clearer. More critically, the keys array is stored directly in the Tags instance without copying after sorting; since it was freshly allocated from toArray this is safe, but it is worth noting for consistency with the immutability goal. libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java [221-231] public static Tags fromMap(Map<String, ?> map) { if (map == null || map.isEmpty()) return EMPTY; - String[] keys = map.keySet().toArray(new String[0]); - Arrays.sort(keys); + String[] keys = map.keySet().stream() + .map(k -> Objects.requireNonNull(k, "map key must not be null")) + .sorted() + .toArray(String[]::new); Object[] values = new Object[keys.length]; for (int i = 0; i < keys.length; i++) { values[i] = Objects.requireNonNull(map.get(keys[i]), "value for key '" + keys[i] + "' must not be null"); validateValue(values[i]); } return new Tags(keys, values, computeHash(keys, values)); } Suggestion importance[1-10]: 3 __ Why: The suggestion adds null-key checking for fromMap, but the method signature Map<String, ?> already enforces String keys at compile time. The ArrayStoreException scenario is only possible with unchecked casts, making this a low-priority defensive improvement.	Low
Possible issue	Guard against null Tags argument in record getTagsMap() now returns a Map.of(...) / Map.ofEntries(...) instance, which is already unmodifiable and has correct equals/hashCode semantics, so using it as a ConcurrentHashMap key is safe. However, tags could be null here (callers may pass null tags), which would cause a NullPointerException. A null-guard should be added. test/telemetry/src/main/java/org/opensearch/test/telemetry/TestInMemoryHistogram.java [56-59] public synchronized void record(double value, Tags tags) { - Map<String, ?> tagsMap = tags.getTagsMap(); + Map<String, ?> tagsMap = (tags != null) ? tags.getTagsMap() : Map.of(); histogramValueForTags.put(tagsMap, value); } Suggestion importance[1-10]: 4 __ Why: Adding a null-guard for tags in record() is a reasonable defensive measure, but this is test infrastructure code and the Histogram interface contract likely requires non-null Tags. The risk of a NullPointerException in practice is low.	Low

Suggestions up to commit 08b48c1

Category	Suggestion	Impact
Possible issue	Prevent internal array mutation by always copying fromPairs mutates the caller-provided rawKeys and rawValues arrays in-place during sorting and deduplication. When w == count (no duplicates), the original arrays are stored directly in the Tags instance without copying, meaning the caller still holds a reference to the internal arrays and could mutate them. Always copy the arrays before storing them to preserve immutability. libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java [361-391] private static Tags fromPairs(String[] rawKeys, Object[] rawValues, int count) { if (count == 0) return EMPTY; for (int i = 1; i < count; i++) { - ... + String key = rawKeys[i]; + Object val = rawValues[i]; + int j = i - 1; + while (j >= 0 && rawKeys[j].compareTo(key) > 0) { + rawKeys[j + 1] = rawKeys[j]; + rawValues[j + 1] = rawValues[j]; + j--; + } + rawKeys[j + 1] = key; + rawValues[j + 1] = val; } - ... - String[] keys = (w == count) ? rawKeys : Arrays.copyOf(rawKeys, w); - Object[] values = (w == count) ? rawValues : Arrays.copyOf(rawValues, w); + + int w = 0; + for (int i = 0; i < count; i++) { + if (w > 0 && rawKeys[w - 1].equals(rawKeys[i])) { + rawValues[w - 1] = rawValues[i]; + } else { + rawKeys[w] = rawKeys[i]; + rawValues[w] = rawValues[i]; + w++; + } + } + + String[] keys = Arrays.copyOf(rawKeys, w); + Object[] values = Arrays.copyOf(rawValues, w); return new Tags(keys, values, computeHash(keys, values)); } Suggestion importance[1-10]: 6 __ Why: The fromPairs method mutates caller-provided arrays and when w == count, stores the original array directly in the Tags instance. However, in practice all callers pass freshly-created inline arrays (e.g., new String[] { k1, k2, k3 }), so external mutation is not a real risk. The suggestion is valid for correctness/immutability guarantees but has low practical impact.	Low
General	Use Tags directly as map key instead of derived map getTagsMap() returns a new Map.ofEntries(...) instance on every call, so two Tags instances with identical content will produce different Map objects that are equal by value but not the same reference. Using these maps as ConcurrentHashMap keys is correct for equality-based lookup, but callers retrieving values via getHistogramValueForTags().get(someMap) must ensure they use an equal map. Consider using the Tags object itself as the key (since it now has correct equals/hashCode) to avoid creating intermediate map objects and to make the intent clearer. test/telemetry/src/main/java/org/opensearch/test/telemetry/TestInMemoryHistogram.java [56-59] -public synchronized void record(double value, Tags tags) { - Map<String, ?> tagsMap = tags.getTagsMap(); - histogramValueForTags.put(tagsMap, value); +private ConcurrentHashMap<Tags, Double> histogramValueForTags = new ConcurrentHashMap<>(); + +public ConcurrentHashMap<Tags, Double> getHistogramValueForTags() { + return this.histogramValueForTags; } +@Override +public synchronized void record(double value, Tags tags) { + histogramValueForTags.put(tags, value); +} + Suggestion importance[1-10]: 5 __ Why: Using Tags directly as the map key is cleaner since Tags now has proper equals/hashCode, avoids creating intermediate Map objects, and makes the intent clearer. However, the current approach using Map<String, ?> as key also works correctly for equality-based lookup, so this is a moderate improvement rather than a critical fix.	Low
	Use consistent hash computation for EMPTY singleton The hashCode for EMPTY is hardcoded as 1, but computeHash on empty arrays returns 1 (since result starts at 1 and no iterations occur). While this happens to be correct today, it creates a fragile implicit dependency. Call computeHash(EMPTY_KEYS, EMPTY_VALUES) explicitly to keep it consistent and self-documenting. libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java [31] -public static Tags EMPTY = new Tags(EMPTY_KEYS, EMPTY_VALUES, 1); +public static final Tags EMPTY = new Tags(EMPTY_KEYS, EMPTY_VALUES, computeHash(EMPTY_KEYS, EMPTY_VALUES)); Suggestion importance[1-10]: 4 __ Why: While the hardcoded 1 happens to be correct (since computeHash on empty arrays returns 1), using computeHash(EMPTY_KEYS, EMPTY_VALUES) explicitly is more maintainable and self-documenting. However, this is a minor style/maintainability improvement with no functional impact.	Low

Suggestions up to commit 078f77f

Category	Suggestion	Impact
Possible issue	Always copy arrays to preserve immutability When w == count the method reuses the caller-supplied rawKeys/rawValues arrays directly as the internal storage of the new Tags instance. Since fromPairs mutates these arrays in-place (insertion sort + dedup), and the caller may retain a reference to the original arrays (e.g. the vararg array passed to of(k1,v1,k2,v2,k3,v3,...)), this breaks the immutability guarantee. Always copy the arrays before storing them. libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java [388-390] -String[] keys = (w == count) ? rawKeys : Arrays.copyOf(rawKeys, w); -Object[] values = (w == count) ? rawValues : Arrays.copyOf(rawValues, w); +String[] keys = Arrays.copyOf(rawKeys, w); +Object[] values = Arrays.copyOf(rawValues, w); return new Tags(keys, values, computeHash(keys, values)); Suggestion importance[1-10]: 7 __ Why: When w == count, the method reuses the caller-supplied arrays directly, which could break immutability if the caller retains a reference to the original arrays. Always copying ensures the immutability guarantee is upheld, though in practice the current callers pass freshly created arrays.	Medium
General	Use Tags directly as map key getTagsMap() now returns a Map.of(...) / Map.ofEntries(...) instance whose equals/hashCode is based on map entry equality. Two separate calls to getTagsMap() for the same Tags will produce equal maps, so lookup by a freshly obtained map will work correctly. However, the ConcurrentHashMap key is now a different object on every call, which may cause subtle issues if callers compare keys by identity. Consider using the Tags object itself as the map key (since it now properly implements equals/hashCode) to avoid creating intermediate map objects and to make the intent clearer. test/telemetry/src/main/java/org/opensearch/test/telemetry/TestInMemoryHistogram.java [56-59] -public synchronized void record(double value, Tags tags) { - Map<String, ?> tagsMap = tags.getTagsMap(); - histogramValueForTags.put(tagsMap, value); +private ConcurrentHashMap<Tags, Double> histogramValueForTags = new ConcurrentHashMap<>(); + +public ConcurrentHashMap<Tags, Double> getHistogramValueForTags() { + return this.histogramValueForTags; } +@Override +public synchronized void record(double value, Tags tags) { + histogramValueForTags.put(tags, value); +} + Suggestion importance[1-10]: 5 __ Why: Using Tags directly as the map key avoids creating intermediate map objects and leverages the new equals/hashCode implementation. However, this requires changing the field type and return type of getHistogramValueForTags(), which may affect callers of that method in the test infrastructure.	Low
	Use consistent hash computation for EMPTY singleton The hashCode for EMPTY is hardcoded as 1, but computeHash on empty arrays returns 1 (since result starts at 1 and no iterations occur). While the value happens to be correct, it is fragile and misleading. Use computeHash(EMPTY_KEYS, EMPTY_VALUES) to make the intent explicit and keep it consistent with all other construction paths. libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java [31] -public static Tags EMPTY = new Tags(EMPTY_KEYS, EMPTY_VALUES, 1); +public static final Tags EMPTY = new Tags(EMPTY_KEYS, EMPTY_VALUES, computeHash(EMPTY_KEYS, EMPTY_VALUES)); Suggestion importance[1-10]: 4 __ Why: While the hardcoded 1 happens to be correct (since computeHash on empty arrays returns 1), using computeHash(EMPTY_KEYS, EMPTY_VALUES) makes the intent explicit and avoids fragility if the hash algorithm changes. This is a minor maintainability improvement.	Low
	Document sorted-keys precondition for merge correctness The concat method assumes both input Tags instances have their keys in sorted order, which is required for the merge to be correct. However, this invariant is not documented or enforced at the call site. If a Tags object were ever constructed with unsorted keys (e.g. via a future code path), the merge would silently produce incorrect results. Add an assertion or at least a comment stating the precondition that both a.keys and b.keys must be sorted. libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java [160-162] public static Tags concat(Tags a, Tags b) { if (a == null || a.keys.length == 0) return (b != null) ? b : EMPTY; if (b == null || b.keys.length == 0) return a; + // Precondition: a.keys and b.keys must each be sorted in ascending order. Suggestion importance[1-10]: 2 __ Why: This suggestion only adds a comment to document an existing precondition. While useful for maintainability, it has minimal functional impact and the improved_code only adds a comment line without any behavioral change.	Low

Suggestions up to commit fa2424a

Category	Suggestion	Impact
Possible issue	Always copy arrays to preserve immutability guarantee When w == count the method reuses the caller-supplied rawKeys/rawValues arrays directly as the internal storage of the new Tags instance. Because fromPairs mutates those arrays in-place (insertion sort + dedup), and the caller may still hold a reference to the original array (e.g. the vararg array passed by of(k1,v1,k2,v2,k3,v3)), this breaks the immutability guarantee. Always copy the arrays before storing them. libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java [402-404] -String[] keys = (w == count) ? rawKeys : Arrays.copyOf(rawKeys, w); -Object[] values = (w == count) ? rawValues : Arrays.copyOf(rawValues, w); +String[] keys = Arrays.copyOf(rawKeys, w); +Object[] values = Arrays.copyOf(rawValues, w); return new Tags(keys, values, computeHash(keys, values)); Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that when w == count, the method reuses the caller-supplied arrays directly, which could break immutability if the caller retains a reference to those arrays. However, in practice the arrays are created inline (e.g., new String[] { k1, k2, k3 }) in the calling of() methods, so the caller doesn't retain a reference. The risk is real for future code changes though, making this a valid defensive improvement.	Medium
General	Use Tags directly as map key for efficiency getTagsMap() now returns Map.ofEntries(...) which creates a new Map instance on every call. Two logically equal Tags objects will produce different Map instances that are not equal by reference, but Map.of/Map.ofEntries does implement value-based equals/hashCode, so lookup should still work. However, using the Tags object itself as the map key would be more efficient and correct since Tags now properly implements equals and hashCode. test/telemetry/src/main/java/org/opensearch/test/telemetry/TestInMemoryHistogram.java [56-59] -public synchronized void record(double value, Tags tags) { - Map<String, ?> tagsMap = tags.getTagsMap(); - histogramValueForTags.put(tagsMap, value); +private ConcurrentHashMap<Tags, Double> histogramValueForTags = new ConcurrentHashMap<>(); + +public ConcurrentHashMap<Tags, Double> getHistogramValueForTags() { + return this.histogramValueForTags; } +@Override +public synchronized void record(double value, Tags tags) { + histogramValueForTags.put(tags, value); +} + Suggestion importance[1-10]: 5 __ Why: Using Tags directly as the map key is more efficient since it avoids creating a new Map instance on every record call, and Tags now properly implements equals/hashCode. However, this requires changing the field type and the getHistogramValueForTags() return type, which may affect callers of that method.	Low
	Document sorted-keys precondition for concat correctness The concat method assumes that the internal keys arrays of both Tags instances are already sorted, which is true for instances created by the public API. However, there is no enforcement of this invariant at the constructor level. If a caller somehow constructs a Tags with unsorted keys (e.g., via reflection or future internal changes), the merge-sort in concat will silently produce incorrect results. Consider adding an assertion or a comment documenting this precondition clearly. libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java [160-162] public static Tags concat(Tags a, Tags b) { + // Precondition: both a.keys and b.keys must be sorted (guaranteed by all factory methods). if (a == null || a.keys.length == 0) return (b != null) ? b : EMPTY; if (b == null || b.keys.length == 0) return a; Suggestion importance[1-10]: 2 __ Why: This suggestion only adds a comment to document a precondition, which is a minor documentation improvement. The constructor is private so external callers cannot violate the invariant, making this a low-impact change.	Low

Suggestions up to commit aa323b0

Category	Suggestion	Impact
Possible issue	Always copy arrays to guarantee immutability When w == count and no deduplication occurred, fromPairs reuses the caller-supplied rawKeys/rawValues arrays directly as the internal storage of the new Tags instance. Since Tags is supposed to be immutable, but the caller still holds a reference to those arrays (e.g., the new String[]{ k1, k2, k3 } literal passed from of(...)), external mutation of those arrays would silently corrupt the Tags state. Always copy the arrays to guarantee true immutability. libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java [375-405] -/** Insertion-sorts by key, deduplicates (last value wins). Mutates the provided arrays. */ -private static Tags fromPairs(String[] rawKeys, Object[] rawValues, int count) { - if (count == 0) return EMPTY; +String[] keys = Arrays.copyOf(rawKeys, w); +Object[] values = Arrays.copyOf(rawValues, w); +return new Tags(keys, values, computeHash(keys, values)); - for (int i = 1; i < count; i++) { - ... - } - - int w = 0; - for (int i = 0; i < count; i++) { - ... - } - - String[] keys = (w == count) ? rawKeys : Arrays.copyOf(rawKeys, w); - Object[] values = (w == count) ? rawValues : Arrays.copyOf(rawValues, w); - return new Tags(keys, values, computeHash(keys, values)); -} - Suggestion importance[1-10]: 6 __ Why: This is a valid immutability concern - when w == count, the internal arrays are shared with the caller's literal arrays. While in practice the callers in of(...) create fresh array literals that aren't retained elsewhere, the defensive copy ensures true immutability regardless of how fromPairs is called in the future.	Low
	Include array length in hash computation The computeHash method does not include the array length in the hash computation, which can lead to hash collisions between Tags instances with different numbers of entries but the same prefix (e.g., Tags.of("a","1") and Tags.of("a","1","b","2") could produce the same hash if the extra entries happen to hash to zero). Including the length in the hash seed prevents this class of collision. libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java [365-372] private static int computeHash(String[] keys, Object[] values) { - int result = 1; + int result = 31 + keys.length; for (int i = 0; i < keys.length; i++) { result = 31 * result + keys[i].hashCode(); result = 31 * result + values[i].hashCode(); } return result; } Suggestion importance[1-10]: 5 __ Why: The suggestion is valid - starting with result = 1 means Tags with different sizes but same prefix entries could theoretically collide. Including keys.length in the seed reduces collision probability. However, this is a minor improvement since the existing algorithm already produces different hashes for different-length arrays in practice (extra entries contribute non-zero hash values).	Low
General	Use Tags as map key for reliable lookup Tags.getTagsMap() now returns a Map.of(...) / Map.ofEntries(...) instance, which is unmodifiable but does not override hashCode/equals in a way that is consistent across different Map implementations. Using the Map returned by getTagsMap() as a ConcurrentHashMap key will work correctly for equality, but test lookup code that constructs a plain HashMap with the same entries to look up a value will fail to find it. Consider using the Tags object itself as the map key, since it now has a well-defined equals/hashCode. test/telemetry/src/main/java/org/opensearch/test/telemetry/TestInMemoryHistogram.java [56-59] -public synchronized void record(double value, Tags tags) { - Map<String, ?> tagsMap = tags.getTagsMap(); - histogramValueForTags.put(tagsMap, value); +private ConcurrentHashMap<Tags, Double> histogramValueForTags = new ConcurrentHashMap<>(); + +public ConcurrentHashMap<Tags, Double> getHistogramValueForTags() { + return this.histogramValueForTags; } +@Override +public synchronized void record(double value, Tags tags) { + histogramValueForTags.put(tags, value); +} + Suggestion importance[1-10]: 6 __ Why: The suggestion raises a valid concern about using Map instances as map keys in tests - Map.ofEntries instances have correct equals/hashCode per the Map contract, but test code constructing a HashMap to look up values would fail. Using Tags directly as the key leverages its well-defined equals/hashCode and is cleaner.	Low
	Compute EMPTY hash consistently via shared method The EMPTY singleton is initialized with a hardcoded hash of 1, but computeHash on empty arrays returns 1 only coincidentally with the current algorithm. If computeHash is ever changed (e.g., to include length), the hardcoded value will silently diverge, breaking equality checks that short-circuit on hashCode. It should be computed via computeHash for consistency. libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java [31] -public static Tags EMPTY = new Tags(EMPTY_KEYS, EMPTY_VALUES, 1); +public static final Tags EMPTY = new Tags(EMPTY_KEYS, EMPTY_VALUES, computeHash(EMPTY_KEYS, EMPTY_VALUES)); Suggestion importance[1-10]: 4 __ Why: The hardcoded 1 for EMPTY's hash happens to match computeHash on empty arrays (since the loop doesn't execute and result stays 1), so this is currently correct. However, using computeHash would make it more maintainable and resilient to future changes in the hash algorithm.	Low

[github-actions]

❌ Gradle check result for 94a2dab: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit cf4e273

[github-actions]

Persistent review updated to latest commit 08b48c1

[github-actions]

✅ Gradle check result for 08b48c1: SUCCESS

[github-actions]

Persistent review updated to latest commit ed54255

[github-actions]

✅ Gradle check result for ed54255: SUCCESS

[msfroh]

@reta -- It looks like @sakrah made the type validation a runtime thing w/ instanceof checks. What are your thoughts on that?

I was kind of thinking of limiting the of methods to a single pair, with overloads for the four valid value types. (As mentioned previously, we can still keep ofStringPairs, since the (String, String) case should be common enough that people would want a convenience method.)

[github-actions]

Persistent review updated to latest commit 47bebbd

[github-actions]

❕ Gradle check result for 47bebbd: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[reta]

@reta -- It looks like @sakrah made the type validation a runtime thing w/ instanceof checks. What are your thoughts on that?

Sorry a bit late, but I think runtime is fine, thank you @msfroh and @sakrah

[msfroh]

@reta -- It looks like @sakrah made the type validation a runtime thing w/ instanceof checks. What are your thoughts on that?

Sorry a bit late, but I think runtime is fine, thank you @msfroh and @sakrah

Surprise! The checks moved back to compile-time. 😁