From cf4e273473a5258dffb2e5055b459ef6e4f170a9 Mon Sep 17 00:00:00 2001 From: Sam Akrah Date: Thu, 5 Mar 2026 14:33:37 -0800 Subject: [PATCH 01/12] Refactor Tags to immutable sorted-array implementation with precomputed hash Replace the mutable HashMap-backed Tags with an immutable design using sorted parallel arrays and a precomputed hashCode. Adds allocation-efficient factories (Tags.of, Tags.concat, Tags.fromMap, Tags.toMap), an EMPTY singleton, and content-based equals/hashCode so Tags can be safely used as map keys, stored in fields, and shared across threads. Deprecated API (create(), addTag()) preserved for backward compatibility; addTag() now returns a new instance instead of mutating in place. Fixes AutoForceMergeMetrics which called addTag() without reassigning the return value -- a silent no-op now that Tags is immutable. Signed-off-by: Sam Akrah Made-with: Cursor Signed-off-by: Sam Akrah --- CHANGELOG.md | 2 + .../telemetry/metrics/tags/Tags.java | 342 +++++++++++++++--- .../telemetry/metrics/tags/TagsTests.java | 334 +++++++++++++++++ .../autoforcemerge/AutoForceMergeMetrics.java | 4 +- 4 files changed, 627 insertions(+), 55 deletions(-) create mode 100644 libs/telemetry/src/test/java/org/opensearch/telemetry/metrics/tags/TagsTests.java diff --git a/CHANGELOG.md b/CHANGELOG.md index 6d9c289e69071..85964c8ce3945 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,12 +39,14 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Implement FieldMappingIngestionMessageMapper for pull-based ingestion ([#20729](https://github.com/opensearch-project/OpenSearch/pull/20729)) ### Changed +- Make telemetry `Tags` immutable ([#20788](https://github.com/opensearch-project/OpenSearch/pull/20788)) - Move Randomness from server to libs/common ([#20570](https://github.com/opensearch-project/OpenSearch/pull/20570)) - Use env variable (OPENSEARCH_FIPS_MODE) to enable opensearch to run in FIPS enforced mode instead of checking for existence of bcFIPS jars ([#20625](https://github.com/opensearch-project/OpenSearch/pull/20625)) - Update streaming flag to use search request context ([#20530](https://github.com/opensearch-project/OpenSearch/pull/20530)) - Move pull-based ingestion classes from experimental to publicAPI ([#20704](https://github.com/opensearch-project/OpenSearch/pull/20704)) ### Fixed +- Fix `AutoForceMergeMetrics` silently dropping tags due to unreassigned `addTag()` return value ([#20788](https://github.com/opensearch-project/OpenSearch/pull/20788)) - Fix flaky test failures in ShardsLimitAllocationDeciderIT ([#20375](https://github.com/opensearch-project/OpenSearch/pull/20375)) - Prevent criteria update for context aware indices ([#20250](https://github.com/opensearch-project/OpenSearch/pull/20250)) - Update EncryptedBlobContainer to adhere limits while listing blobs in specific sort order if wrapped blob container supports ([#20514](https://github.com/opensearch-project/OpenSearch/pull/20514)) diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java index f2a8764f8021d..f50e903b16d9d 100644 --- a/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java +++ b/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java @@ -10,90 +10,326 @@ import org.opensearch.common.annotation.ExperimentalApi; +import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.Map; import java.util.Objects; /** - * Class to create tags for a meter. + * Immutable tags for a meter. * * @opensearch.experimental */ @ExperimentalApi -public class Tags { - private final Map tagsMap; - /** - * Empty value. - */ - public final static Tags EMPTY = new Tags(Collections.emptyMap()); +public final class Tags { + + private static final String[] EMPTY_KEYS = new String[0]; + private static final Object[] EMPTY_VALUES = new Object[0]; + + public static final Tags EMPTY = new Tags(EMPTY_KEYS, EMPTY_VALUES, 1); + + private final String[] keys; + private final Object[] values; + private final int hashCode; + + private Tags(String[] keys, Object[] values, int hashCode) { + this.keys = keys; + this.values = values; + this.hashCode = hashCode; + } + + // ----------------------------------------------------------------------- + // Factories + // ----------------------------------------------------------------------- + + /** Creates an immutable Tags with one key-value pair. */ + public static Tags of(String key, Object value) { + Objects.requireNonNull(key, "key must not be null"); + Objects.requireNonNull(value, "value must not be null"); + String[] k = { key }; + Object[] v = { value }; + return new Tags(k, v, computeHash(k, v)); + } + + /** Creates an immutable Tags with two key-value pairs. */ + public static Tags of(String k1, Object v1, String k2, Object v2) { + 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"); + 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)); + } + + /** Creates an immutable Tags with three key-value pairs. */ + public static Tags of(String k1, Object v1, String k2, Object v2, String k3, Object v3) { + 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"); + Objects.requireNonNull(k3, "k3 must not be null"); + Objects.requireNonNull(v3, "v3 must not be null"); + return fromPairs(new String[] { k1, k2, k3 }, new Object[] { v1, v2, v3 }, 3); + } + + /** Creates an immutable Tags with four key-value pairs. */ + public static Tags of(String k1, Object v1, String k2, Object v2, String k3, Object v3, String k4, Object v4) { + 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"); + Objects.requireNonNull(k3, "k3 must not be null"); + Objects.requireNonNull(v3, "v3 must not be null"); + Objects.requireNonNull(k4, "k4 must not be null"); + Objects.requireNonNull(v4, "v4 must not be null"); + return fromPairs(new String[] { k1, k2, k3, k4 }, new Object[] { v1, v2, v3, v4 }, 4); + } /** - * Factory method. - * @return tags. + * Creates Tags from interleaved key-value pairs; must be even length. */ - public static Tags create() { - return new Tags(new HashMap<>()); + public static Tags of(String... keyValues) { + if (keyValues == null || keyValues.length == 0) return EMPTY; + if (keyValues.length % 2 != 0) { + throw new IllegalArgumentException("keyValues must be even length, got " + keyValues.length); + } + int count = keyValues.length / 2; + String[] keys = new String[count]; + Object[] values = new Object[count]; + for (int i = 0; i < count; i++) { + keys[i] = Objects.requireNonNull(keyValues[i * 2], "key at index " + (i * 2) + " must not be null"); + values[i] = Objects.requireNonNull(keyValues[i * 2 + 1], "value at index " + (i * 2 + 1) + " must not be null"); + } + return fromPairs(keys, values, count); } /** - * Constructor. + * Merges two Tags. On key collision, {@code b} wins. Either argument may be null. */ - private Tags(Map tagsMap) { - this.tagsMap = tagsMap; + 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; + + int thisLength = a.keys.length; + int otherLength = b.keys.length; + String[] mergedKeys = new String[thisLength + otherLength]; + Object[] mergedValues = new Object[thisLength + otherLength]; + int thisIndex = 0, otherIndex = 0, sortedIndex = 0; + + while (thisIndex < thisLength && otherIndex < otherLength) { + int cmp = a.keys[thisIndex].compareTo(b.keys[otherIndex]); + if (cmp < 0) { + mergedKeys[sortedIndex] = a.keys[thisIndex]; + mergedValues[sortedIndex] = a.values[thisIndex]; + thisIndex++; + } else if (cmp > 0) { + mergedKeys[sortedIndex] = b.keys[otherIndex]; + mergedValues[sortedIndex] = b.values[otherIndex]; + otherIndex++; + } else { + mergedKeys[sortedIndex] = b.keys[otherIndex]; + mergedValues[sortedIndex] = b.values[otherIndex]; + thisIndex++; + otherIndex++; + } + sortedIndex++; + } + int thisRemaining = thisLength - thisIndex; + if (thisRemaining > 0) { + System.arraycopy(a.keys, thisIndex, mergedKeys, sortedIndex, thisRemaining); + System.arraycopy(a.values, thisIndex, mergedValues, sortedIndex, thisRemaining); + sortedIndex += thisRemaining; + } + int otherRemaining = otherLength - otherIndex; + if (otherRemaining > 0) { + System.arraycopy(b.keys, otherIndex, mergedKeys, sortedIndex, otherRemaining); + System.arraycopy(b.values, otherIndex, mergedValues, sortedIndex, otherRemaining); + sortedIndex += otherRemaining; + } + + String[] keys = (sortedIndex == mergedKeys.length) ? mergedKeys : Arrays.copyOf(mergedKeys, sortedIndex); + Object[] values = (sortedIndex == mergedValues.length) ? mergedValues : Arrays.copyOf(mergedValues, sortedIndex); + return new Tags(keys, values, computeHash(keys, values)); + } + + /** Creates Tags from a map. */ + public static Tags fromMap(Map map) { + if (map == null || map.isEmpty()) return EMPTY; + String[] keys = map.keySet().toArray(new String[0]); + Arrays.sort(keys); + 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"); + } + return new Tags(keys, values, computeHash(keys, values)); + } + + // ----------------------------------------------------------------------- + // Accessors + // ----------------------------------------------------------------------- + + public int size() { + return keys.length; + } + + public String getKey(int i) { + return keys[i]; + } + + public Object getValue(int i) { + return values[i]; } /** - * Add String attribute. - * @param key key - * @param value value - * @return Same instance. + * Converts to a String-valued map. Intended for flush-time, not the hot path. */ + public Map toMap() { + if (keys.length == 0) return Collections.emptyMap(); + Map map = new HashMap<>(keys.length); + for (int i = 0; i < keys.length; i++) { + map.put(keys[i], String.valueOf(values[i])); + } + return map; + } + + /** Returns an unmodifiable map preserving original value types. */ + public Map getTagsMap() { + if (keys.length == 0) return Collections.emptyMap(); + Map map = new HashMap<>(keys.length); + for (int i = 0; i < keys.length; i++) { + map.put(keys[i], values[i]); + } + return Collections.unmodifiableMap(map); + } + + // ----------------------------------------------------------------------- + // equals / hashCode / toString + // ----------------------------------------------------------------------- + + @Override + public int hashCode() { + return hashCode; + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (!(o instanceof Tags that)) return false; + if (this.hashCode != that.hashCode) return false; + if (this.keys.length != that.keys.length) return false; + for (int i = 0; i < keys.length; i++) { + if (!keys[i].equals(that.keys[i])) return false; + if (!values[i].equals(that.values[i])) return false; + } + return true; + } + + @Override + public String toString() { + if (keys.length == 0) return "Tags{}"; + StringBuilder sb = new StringBuilder("Tags{"); + for (int i = 0; i < keys.length; i++) { + if (i > 0) sb.append(", "); + sb.append(keys[i]).append('=').append(values[i]); + } + return sb.append('}').toString(); + } + + // ----------------------------------------------------------------------- + // Deprecated API + // ----------------------------------------------------------------------- + + /** @deprecated Use {@link #EMPTY} instead. */ + @Deprecated + public static Tags create() { + return EMPTY; + } + + /** @deprecated Use {@link #of} or {@link #concat} instead. */ + @Deprecated public Tags addTag(String key, String value) { + Objects.requireNonNull(key, "key cannot be null"); Objects.requireNonNull(value, "value cannot be null"); - tagsMap.put(key, value); - return this; + return Tags.concat(this, Tags.of(key, (Object) value)); } - /** - * Add long attribute. - * @param key key - * @param value value - * @return Same instance. - */ + /** @deprecated Use {@link #of} or {@link #concat} instead. */ + @Deprecated public Tags addTag(String key, long value) { - tagsMap.put(key, value); - return this; - }; + Objects.requireNonNull(key, "key cannot be null"); + return Tags.concat(this, Tags.of(key, (Object) value)); + } - /** - * Add double attribute. - * @param key key - * @param value value - * @return Same instance. - */ + /** @deprecated Use {@link #of} or {@link #concat} instead. */ + @Deprecated public Tags addTag(String key, double value) { - tagsMap.put(key, value); - return this; - }; + Objects.requireNonNull(key, "key cannot be null"); + return Tags.concat(this, Tags.of(key, (Object) value)); + } - /** - * Add boolean attribute. - * @param key key - * @param value value - * @return Same instance. - */ + /** @deprecated Use {@link #of} or {@link #concat} instead. */ + @Deprecated public Tags addTag(String key, boolean value) { - tagsMap.put(key, value); - return this; - }; + Objects.requireNonNull(key, "key cannot be null"); + return Tags.concat(this, Tags.of(key, (Object) value)); + } - /** - * Returns the attribute map. - * @return tags map - */ - public Map getTagsMap() { - return Collections.unmodifiableMap(tagsMap); + // ----------------------------------------------------------------------- + // Internal + // ----------------------------------------------------------------------- + + private static int computeHash(String[] keys, Object[] values) { + int result = 1; + for (int i = 0; i < keys.length; i++) { + result = 31 * result + keys[i].hashCode(); + result = 31 * result + values[i].hashCode(); + } + return result; } + /** 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; + + 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)); + } } diff --git a/libs/telemetry/src/test/java/org/opensearch/telemetry/metrics/tags/TagsTests.java b/libs/telemetry/src/test/java/org/opensearch/telemetry/metrics/tags/TagsTests.java new file mode 100644 index 0000000000000..35114d03e4fdd --- /dev/null +++ b/libs/telemetry/src/test/java/org/opensearch/telemetry/metrics/tags/TagsTests.java @@ -0,0 +1,334 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.telemetry.metrics.tags; + +import org.opensearch.test.OpenSearchTestCase; + +import java.util.HashMap; +import java.util.Map; + +public class TagsTests extends OpenSearchTestCase { + + // --- EMPTY --- + + public void testEmptyHasZeroSize() { + assertEquals(0, Tags.EMPTY.size()); + } + + public void testEmptyToMapReturnsEmptyMap() { + assertTrue(Tags.EMPTY.toMap().isEmpty()); + } + + public void testEmptyGetTagsMapReturnsEmptyMap() { + assertTrue(Tags.EMPTY.getTagsMap().isEmpty()); + } + + // --- of(key, value) --- + + public void testOfSingleTag() { + Tags t = Tags.of("env", "prod"); + assertEquals(1, t.size()); + assertEquals("env", t.getKey(0)); + assertEquals("prod", t.getValue(0)); + } + + public void testOfNullKeyThrows() { + expectThrows(NullPointerException.class, () -> Tags.of(null, "v")); + } + + public void testOfNullValueThrows() { + expectThrows(NullPointerException.class, () -> Tags.of("k", (Object) null)); + } + + // --- of(k1,v1,k2,v2) sorted --- + + public void testOfTwoTagsSorted() { + Tags t = Tags.of("z", "1", "a", "2"); + assertEquals(2, t.size()); + assertEquals("a", t.getKey(0)); + assertEquals("z", t.getKey(1)); + } + + public void testOfTwoTagsDuplicateKeyLastWins() { + Tags t = Tags.of("k", "first", "k", "second"); + assertEquals(1, t.size()); + assertEquals("second", t.getValue(0)); + } + + // --- of(k1,v1,k2,v2,k3,v3) --- + + public void testOfThreeTagsSorted() { + Tags t = Tags.of("c", "3", "a", "1", "b", "2"); + assertEquals(3, t.size()); + assertEquals("a", t.getKey(0)); + assertEquals("b", t.getKey(1)); + assertEquals("c", t.getKey(2)); + } + + // --- of(varargs) --- + + public void testOfVarargsEmpty() { + assertSame(Tags.EMPTY, Tags.of(new String[0])); + } + + public void testOfVarargsOddLengthThrows() { + expectThrows(IllegalArgumentException.class, () -> Tags.of("a", "b", "c")); + } + + public void testOfVarargsSorted() { + Tags t = Tags.of("z", "1", "a", "2", "m", "3"); + assertEquals(3, t.size()); + assertEquals("a", t.getKey(0)); + assertEquals("m", t.getKey(1)); + assertEquals("z", t.getKey(2)); + } + + // --- concat --- + + public void testConcatMergesTwoTags() { + Tags a = Tags.of("a", "1"); + Tags b = Tags.of("b", "2"); + Tags merged = Tags.concat(a, b); + assertEquals(2, merged.size()); + assertEquals("a", merged.getKey(0)); + assertEquals("b", merged.getKey(1)); + } + + public void testConcatBWinsOnCollision() { + Tags a = Tags.of("k", "old"); + Tags b = Tags.of("k", "new"); + Tags merged = Tags.concat(a, b); + assertEquals(1, merged.size()); + assertEquals("new", merged.getValue(0)); + } + + public void testConcatWithNullReturnsOther() { + Tags a = Tags.of("k", "v"); + assertSame(a, Tags.concat(a, null)); + assertSame(a, Tags.concat(null, a)); + } + + public void testConcatWithEmptyReturnsOther() { + Tags a = Tags.of("k", "v"); + assertSame(a, Tags.concat(a, Tags.EMPTY)); + assertSame(a, Tags.concat(Tags.EMPTY, a)); + } + + public void testConcatBothNullReturnsEmpty() { + assertSame(Tags.EMPTY, Tags.concat(null, null)); + } + + public void testConcatPartialOverlapMergesAndDeduplicates() { + Tags a = Tags.of("a", "1", "c", "3"); + Tags b = Tags.of("b", "2", "c", "4"); + Tags merged = Tags.concat(a, b); + assertEquals(3, merged.size()); + assertEquals("a", merged.getKey(0)); + assertEquals("1", merged.getValue(0)); + assertEquals("b", merged.getKey(1)); + assertEquals("2", merged.getValue(1)); + assertEquals("c", merged.getKey(2)); + assertEquals("4", merged.getValue(2)); + } + + public void testConcatInterleavedNoOverlap() { + Tags a = Tags.of("a", "1", "c", "3", "e", "5"); + Tags b = Tags.of("b", "2", "d", "4"); + Tags merged = Tags.concat(a, b); + assertEquals(5, merged.size()); + assertEquals("a", merged.getKey(0)); + assertEquals("b", merged.getKey(1)); + assertEquals("c", merged.getKey(2)); + assertEquals("d", merged.getKey(3)); + assertEquals("e", merged.getKey(4)); + } + + public void testConcatFullOverlapBWins() { + Tags a = Tags.of("a", "old_a", "b", "old_b"); + Tags b = Tags.of("a", "new_a", "b", "new_b"); + Tags merged = Tags.concat(a, b); + assertEquals(2, merged.size()); + assertEquals("new_a", merged.getValue(0)); + assertEquals("new_b", merged.getValue(1)); + } + + public void testConcatLargeRemainderPath() { + Tags a = Tags.of("z", "26"); + Tags b = Tags.of("a", "1", "b", "2", "c", "3", "d", "4"); + Tags merged = Tags.concat(a, b); + assertEquals(5, merged.size()); + assertEquals("a", merged.getKey(0)); + assertEquals("d", merged.getKey(3)); + assertEquals("z", merged.getKey(4)); + assertEquals("26", merged.getValue(4)); + } + + public void testConcatHashConsistency() { + Tags viaOf = Tags.of("a", "1", "b", "2"); + Tags viaConcat = Tags.concat(Tags.of("a", "1"), Tags.of("b", "2")); + assertEquals(viaOf, viaConcat); + assertEquals(viaOf.hashCode(), viaConcat.hashCode()); + } + + public void testConcatResultIsSorted() { + Tags a = Tags.of("x", "1"); + Tags b = Tags.of("a", "2", "m", "3"); + Tags merged = Tags.concat(a, b); + for (int i = 0; i < merged.size() - 1; i++) { + assertTrue(merged.getKey(i).compareTo(merged.getKey(i + 1)) < 0); + } + } + + // --- fromMap --- + + public void testFromMapRoundTrips() { + Map map = new HashMap<>(); + map.put("b", "2"); + map.put("a", "1"); + Tags t = Tags.fromMap(map); + assertEquals(2, t.size()); + assertEquals("a", t.getKey(0)); + assertEquals("b", t.getKey(1)); + assertEquals("1", t.getValue(0)); + assertEquals("2", t.getValue(1)); + } + + public void testFromMapNullReturnsEmpty() { + assertSame(Tags.EMPTY, Tags.fromMap(null)); + } + + public void testFromMapEmptyReturnsEmpty() { + assertSame(Tags.EMPTY, Tags.fromMap(Map.of())); + } + + public void testFromMapNullValueThrows() { + Map map = new HashMap<>(); + map.put("k", null); + expectThrows(NullPointerException.class, () -> Tags.fromMap(map)); + } + + // --- toMap --- + + public void testToMapConvertsValuesToStrings() { + Tags t = Tags.of("k", (Object) 42L); + Map map = t.toMap(); + assertEquals("42", map.get("k")); + } + + public void testToMapWithStringValues() { + Tags t = Tags.of("a", "1", "b", "2"); + Map map = t.toMap(); + assertEquals("1", map.get("a")); + assertEquals("2", map.get("b")); + } + + // --- getTagsMap --- + + public void testGetTagsMapPreservesOriginalTypes() { + Tags t = Tags.of("num", (Object) 42L); + assertEquals(42L, t.getTagsMap().get("num")); + } + + // --- equals / hashCode --- + + public void testEqualTagsAreEqual() { + Tags a = Tags.of("x", "1", "y", "2"); + Tags b = Tags.of("x", "1", "y", "2"); + assertEquals(a, b); + assertEquals(a.hashCode(), b.hashCode()); + } + + public void testDifferentTagsAreNotEqual() { + Tags a = Tags.of("x", "1"); + Tags b = Tags.of("x", "2"); + assertNotEquals(a, b); + } + + public void testEmptyEqualsEmpty() { + assertEquals(Tags.EMPTY, Tags.create()); + } + + public void testIdentityEquals() { + Tags t = Tags.of("k", "v"); + assertEquals(t, t); + } + + public void testDifferentSizeNotEqual() { + Tags a = Tags.of("k", "v"); + Tags b = Tags.of("k", "v", "k2", "v2"); + assertNotEquals(a, b); + } + + // --- toString --- + + public void testToStringEmpty() { + assertEquals("Tags{}", Tags.EMPTY.toString()); + } + + public void testToStringWithTags() { + Tags t = Tags.of("a", "1"); + assertEquals("Tags{a=1}", t.toString()); + } + + // --- Deprecated API backward compatibility --- + + public void testCreateReturnsEmpty() { + assertSame(Tags.EMPTY, Tags.create()); + } + + public void testAddTagReturnsNewInstance() { + Tags original = Tags.create(); + Tags updated = original.addTag("k", "v"); + assertNotSame(original, updated); + assertEquals(0, original.size()); + assertEquals(1, updated.size()); + } + + public void testAddTagChaining() { + Tags t = Tags.create().addTag("a", "1").addTag("b", "2").addTag("c", "3"); + assertEquals(3, t.size()); + assertEquals("a", t.getKey(0)); + assertEquals("b", t.getKey(1)); + assertEquals("c", t.getKey(2)); + } + + public void testAddTagLong() { + Tags t = Tags.create().addTag("num", 42L); + assertEquals(1, t.size()); + assertEquals(42L, t.getValue(0)); + } + + public void testAddTagDouble() { + Tags t = Tags.create().addTag("val", 3.14); + assertEquals(1, t.size()); + assertEquals(3.14, t.getValue(0)); + } + + public void testAddTagBoolean() { + Tags t = Tags.create().addTag("flag", true); + assertEquals(1, t.size()); + assertEquals(true, t.getValue(0)); + } + + public void testAddTagOverwritesPreviousValue() { + Tags t = Tags.create().addTag("k", "old").addTag("k", "new"); + assertEquals(1, t.size()); + assertEquals("new", t.getValue(0)); + } + + // --- Concurrent safety: Tags is immutable so no sharing issues --- + + public void testUsableAsMapKey() { + Tags t1 = Tags.of("k", "v"); + Tags t2 = Tags.of("k", "v"); + Map map = new HashMap<>(); + map.put(t1, "found"); + assertEquals("found", map.get(t2)); + } +} diff --git a/server/src/main/java/org/opensearch/index/autoforcemerge/AutoForceMergeMetrics.java b/server/src/main/java/org/opensearch/index/autoforcemerge/AutoForceMergeMetrics.java index 417835006b7a2..4cbce2d9dc23e 100644 --- a/server/src/main/java/org/opensearch/index/autoforcemerge/AutoForceMergeMetrics.java +++ b/server/src/main/java/org/opensearch/index/autoforcemerge/AutoForceMergeMetrics.java @@ -111,9 +111,9 @@ public Optional getTags(Optional nodeId, Optional shardId) Tags tags = Tags.create(); if (shardId.isPresent()) { - tags.addTag(SHARD_ID, shardId.get()); + tags = tags.addTag(SHARD_ID, shardId.get()); } else if (nodeId.isPresent()) { - tags.addTag(NODE_ID, nodeId.get()); + tags = tags.addTag(NODE_ID, nodeId.get()); } return Optional.of(tags); From cf503ce4dab213a69c8ce2e2acb5fc303b03cd75 Mon Sep 17 00:00:00 2001 From: Sam Akrah Date: Thu, 5 Mar 2026 15:31:15 -0800 Subject: [PATCH 02/12] Addressed javadoc styling Signed-off-by: Sam Akrah --- .../telemetry/metrics/tags/Tags.java | 106 ++++++++++++++++-- 1 file changed, 96 insertions(+), 10 deletions(-) diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java index f50e903b16d9d..208e54be5ed53 100644 --- a/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java +++ b/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java @@ -27,6 +27,9 @@ public final class Tags { private static final String[] EMPTY_KEYS = new String[0]; private static final Object[] EMPTY_VALUES = new Object[0]; + /** + * Empty tags singleton. + */ public static final Tags EMPTY = new Tags(EMPTY_KEYS, EMPTY_VALUES, 1); private final String[] keys; @@ -43,7 +46,12 @@ private Tags(String[] keys, Object[] values, int hashCode) { // Factories // ----------------------------------------------------------------------- - /** Creates an immutable Tags with one key-value pair. */ + /** + * Creates an immutable Tags with one key-value pair. + * @param key tag key + * @param value tag value + * @return new Tags instance + */ public static Tags of(String key, Object value) { Objects.requireNonNull(key, "key must not be null"); Objects.requireNonNull(value, "value must not be null"); @@ -52,7 +60,14 @@ public static Tags of(String key, Object value) { return new Tags(k, v, computeHash(k, v)); } - /** Creates an immutable Tags with two key-value pairs. */ + /** + * Creates an immutable Tags with two key-value pairs. + * @param k1 first key + * @param v1 first value + * @param k2 second key + * @param v2 second value + * @return new Tags instance + */ public static Tags of(String k1, Object v1, String k2, Object v2) { Objects.requireNonNull(k1, "k1 must not be null"); Objects.requireNonNull(v1, "v1 must not be null"); @@ -74,7 +89,16 @@ public static Tags of(String k1, Object v1, String k2, Object v2) { return new Tags(keys, values, computeHash(keys, values)); } - /** Creates an immutable Tags with three key-value pairs. */ + /** + * Creates an immutable Tags with three key-value pairs. + * @param k1 first key + * @param v1 first value + * @param k2 second key + * @param v2 second value + * @param k3 third key + * @param v3 third value + * @return new Tags instance + */ public static Tags of(String k1, Object v1, String k2, Object v2, String k3, Object v3) { Objects.requireNonNull(k1, "k1 must not be null"); Objects.requireNonNull(v1, "v1 must not be null"); @@ -85,7 +109,18 @@ public static Tags of(String k1, Object v1, String k2, Object v2, String k3, Obj return fromPairs(new String[] { k1, k2, k3 }, new Object[] { v1, v2, v3 }, 3); } - /** Creates an immutable Tags with four key-value pairs. */ + /** + * Creates an immutable Tags with four key-value pairs. + * @param k1 first key + * @param v1 first value + * @param k2 second key + * @param v2 second value + * @param k3 third key + * @param v3 third value + * @param k4 fourth key + * @param v4 fourth value + * @return new Tags instance + */ public static Tags of(String k1, Object v1, String k2, Object v2, String k3, Object v3, String k4, Object v4) { Objects.requireNonNull(k1, "k1 must not be null"); Objects.requireNonNull(v1, "v1 must not be null"); @@ -100,6 +135,8 @@ public static Tags of(String k1, Object v1, String k2, Object v2, String k3, Obj /** * Creates Tags from interleaved key-value pairs; must be even length. + * @param keyValues alternating keys and values + * @return new Tags instance */ public static Tags of(String... keyValues) { if (keyValues == null || keyValues.length == 0) return EMPTY; @@ -118,6 +155,9 @@ public static Tags of(String... keyValues) { /** * Merges two Tags. On key collision, {@code b} wins. Either argument may be null. + * @param a first tags + * @param b second tags + * @return merged Tags instance */ public static Tags concat(Tags a, Tags b) { if (a == null || a.keys.length == 0) return (b != null) ? b : EMPTY; @@ -165,7 +205,11 @@ public static Tags concat(Tags a, Tags b) { return new Tags(keys, values, computeHash(keys, values)); } - /** Creates Tags from a map. */ + /** + * Creates Tags from a map. + * @param map key-value pairs + * @return new Tags instance + */ public static Tags fromMap(Map map) { if (map == null || map.isEmpty()) return EMPTY; String[] keys = map.keySet().toArray(new String[0]); @@ -181,14 +225,28 @@ public static Tags fromMap(Map map) { // Accessors // ----------------------------------------------------------------------- + /** + * Returns the number of tags. + * @return tag count + */ public int size() { return keys.length; } + /** + * Returns the key at the given index. + * @param i index + * @return key + */ public String getKey(int i) { return keys[i]; } + /** + * Returns the value at the given index. + * @param i index + * @return value + */ public Object getValue(int i) { return values[i]; } @@ -252,13 +310,23 @@ public String toString() { // Deprecated API // ----------------------------------------------------------------------- - /** @deprecated Use {@link #EMPTY} instead. */ + /** + * Factory method. + * @return tags + * @deprecated Use {@link #EMPTY} instead. + */ @Deprecated public static Tags create() { return EMPTY; } - /** @deprecated Use {@link #of} or {@link #concat} instead. */ + /** + * Add String attribute. + * @param key key + * @param value value + * @return new Tags instance with the added tag + * @deprecated Use {@link #of} or {@link #concat} instead. + */ @Deprecated public Tags addTag(String key, String value) { Objects.requireNonNull(key, "key cannot be null"); @@ -266,21 +334,39 @@ public Tags addTag(String key, String value) { return Tags.concat(this, Tags.of(key, (Object) value)); } - /** @deprecated Use {@link #of} or {@link #concat} instead. */ + /** + * Add long attribute. + * @param key key + * @param value value + * @return new Tags instance with the added tag + * @deprecated Use {@link #of} or {@link #concat} instead. + */ @Deprecated public Tags addTag(String key, long value) { Objects.requireNonNull(key, "key cannot be null"); return Tags.concat(this, Tags.of(key, (Object) value)); } - /** @deprecated Use {@link #of} or {@link #concat} instead. */ + /** + * Add double attribute. + * @param key key + * @param value value + * @return new Tags instance with the added tag + * @deprecated Use {@link #of} or {@link #concat} instead. + */ @Deprecated public Tags addTag(String key, double value) { Objects.requireNonNull(key, "key cannot be null"); return Tags.concat(this, Tags.of(key, (Object) value)); } - /** @deprecated Use {@link #of} or {@link #concat} instead. */ + /** + * Add boolean attribute. + * @param key key + * @param value value + * @return new Tags instance with the added tag + * @deprecated Use {@link #of} or {@link #concat} instead. + */ @Deprecated public Tags addTag(String key, boolean value) { Objects.requireNonNull(key, "key cannot be null"); From 6f1b1ebc14de8ac68ce1bb195c7419c86e623f42 Mon Sep 17 00:00:00 2001 From: Sam Akrah Date: Thu, 5 Mar 2026 19:03:18 -0800 Subject: [PATCH 03/12] retrigger CI Signed-off-by: Sam Akrah From 6e1ff7437fc2917bd786d38aa6e9e7c1419dc98d Mon Sep 17 00:00:00 2001 From: Sam Akrah Date: Fri, 6 Mar 2026 10:59:08 -0800 Subject: [PATCH 04/12] retrigger CI Signed-off-by: Sam Akrah From f8f21dbf050bdd04eee631bc9f44254d4a36c4d8 Mon Sep 17 00:00:00 2001 From: Sam Akrah Date: Fri, 6 Mar 2026 12:35:51 -0800 Subject: [PATCH 05/12] Address review feedback: improve Tags API clarity and compatibility - Rename of(String...) to ofStringPairs(String...) to avoid ambiguity with of(String, Object) overload - Widen fromMap(Map) to fromMap(Map) for flexibility - Remove toMap() in favor of getTagsMap() as the single map accessor - Refactor getTagsMap() to use Map.ofEntries for compact unmodifiable map - Simplify equals() to use Arrays.equals instead of manual loop - Fix TestInMemoryHistogram to use Map instead of HashMap, avoiding ClassCastException with new getTagsMap() return type - Update tests to reflect renamed methods and removed toMap() Signed-off-by: Sam Akrah --- .../telemetry/metrics/tags/Tags.java | 37 ++++++------------- .../telemetry/metrics/tags/TagsTests.java | 33 ++++------------- .../test/telemetry/TestInMemoryHistogram.java | 10 ++--- 3 files changed, 23 insertions(+), 57 deletions(-) diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java index 208e54be5ed53..a8a7b150639c0 100644 --- a/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java +++ b/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java @@ -11,8 +11,6 @@ import org.opensearch.common.annotation.ExperimentalApi; import java.util.Arrays; -import java.util.Collections; -import java.util.HashMap; import java.util.Map; import java.util.Objects; @@ -134,11 +132,11 @@ public static Tags of(String k1, Object v1, String k2, Object v2, String k3, Obj } /** - * Creates Tags from interleaved key-value pairs; must be even length. + * Creates Tags from interleaved String key-value pairs; must be even length. * @param keyValues alternating keys and values * @return new Tags instance */ - public static Tags of(String... keyValues) { + public static Tags ofStringPairs(String... keyValues) { if (keyValues == null || keyValues.length == 0) return EMPTY; if (keyValues.length % 2 != 0) { throw new IllegalArgumentException("keyValues must be even length, got " + keyValues.length); @@ -210,7 +208,7 @@ public static Tags concat(Tags a, Tags b) { * @param map key-value pairs * @return new Tags instance */ - public static Tags fromMap(Map map) { + public static Tags fromMap(Map map) { if (map == null || map.isEmpty()) return EMPTY; String[] keys = map.keySet().toArray(new String[0]); Arrays.sort(keys); @@ -252,25 +250,17 @@ public Object getValue(int i) { } /** - * Converts to a String-valued map. Intended for flush-time, not the hot path. + * Returns an unmodifiable map preserving original value types. + * @return unmodifiable map of tags */ - public Map toMap() { - if (keys.length == 0) return Collections.emptyMap(); - Map map = new HashMap<>(keys.length); - for (int i = 0; i < keys.length; i++) { - map.put(keys[i], String.valueOf(values[i])); - } - return map; - } - - /** Returns an unmodifiable map preserving original value types. */ + @SuppressWarnings({ "unchecked", "rawtypes" }) public Map getTagsMap() { - if (keys.length == 0) return Collections.emptyMap(); - Map map = new HashMap<>(keys.length); + if (keys.length == 0) return Map.of(); + Map.Entry[] entries = new Map.Entry[keys.length]; for (int i = 0; i < keys.length; i++) { - map.put(keys[i], values[i]); + entries[i] = Map.entry(keys[i], values[i]); } - return Collections.unmodifiableMap(map); + return Map.ofEntries(entries); } // ----------------------------------------------------------------------- @@ -287,12 +277,7 @@ public boolean equals(Object o) { if (this == o) return true; if (!(o instanceof Tags that)) return false; if (this.hashCode != that.hashCode) return false; - if (this.keys.length != that.keys.length) return false; - for (int i = 0; i < keys.length; i++) { - if (!keys[i].equals(that.keys[i])) return false; - if (!values[i].equals(that.values[i])) return false; - } - return true; + return Arrays.equals(keys, that.keys) && Arrays.equals(values, that.values); } @Override diff --git a/libs/telemetry/src/test/java/org/opensearch/telemetry/metrics/tags/TagsTests.java b/libs/telemetry/src/test/java/org/opensearch/telemetry/metrics/tags/TagsTests.java index 35114d03e4fdd..83dbf3fc3bce6 100644 --- a/libs/telemetry/src/test/java/org/opensearch/telemetry/metrics/tags/TagsTests.java +++ b/libs/telemetry/src/test/java/org/opensearch/telemetry/metrics/tags/TagsTests.java @@ -21,10 +21,6 @@ public void testEmptyHasZeroSize() { assertEquals(0, Tags.EMPTY.size()); } - public void testEmptyToMapReturnsEmptyMap() { - assertTrue(Tags.EMPTY.toMap().isEmpty()); - } - public void testEmptyGetTagsMapReturnsEmptyMap() { assertTrue(Tags.EMPTY.getTagsMap().isEmpty()); } @@ -71,18 +67,18 @@ public void testOfThreeTagsSorted() { assertEquals("c", t.getKey(2)); } - // --- of(varargs) --- + // --- ofStringPairs(varargs) --- - public void testOfVarargsEmpty() { - assertSame(Tags.EMPTY, Tags.of(new String[0])); + public void testOfStringPairsEmpty() { + assertSame(Tags.EMPTY, Tags.ofStringPairs(new String[0])); } - public void testOfVarargsOddLengthThrows() { - expectThrows(IllegalArgumentException.class, () -> Tags.of("a", "b", "c")); + public void testOfStringPairsOddLengthThrows() { + expectThrows(IllegalArgumentException.class, () -> Tags.ofStringPairs("a", "b", "c")); } - public void testOfVarargsSorted() { - Tags t = Tags.of("z", "1", "a", "2", "m", "3"); + public void testOfStringPairsSorted() { + Tags t = Tags.ofStringPairs("z", "1", "a", "2", "m", "3"); assertEquals(3, t.size()); assertEquals("a", t.getKey(0)); assertEquals("m", t.getKey(1)); @@ -213,21 +209,6 @@ public void testFromMapNullValueThrows() { expectThrows(NullPointerException.class, () -> Tags.fromMap(map)); } - // --- toMap --- - - public void testToMapConvertsValuesToStrings() { - Tags t = Tags.of("k", (Object) 42L); - Map map = t.toMap(); - assertEquals("42", map.get("k")); - } - - public void testToMapWithStringValues() { - Tags t = Tags.of("a", "1", "b", "2"); - Map map = t.toMap(); - assertEquals("1", map.get("a")); - assertEquals("2", map.get("b")); - } - // --- getTagsMap --- public void testGetTagsMapPreservesOriginalTypes() { diff --git a/test/telemetry/src/main/java/org/opensearch/test/telemetry/TestInMemoryHistogram.java b/test/telemetry/src/main/java/org/opensearch/test/telemetry/TestInMemoryHistogram.java index 6028fcb43114f..e8ee8b8eb0fdb 100644 --- a/test/telemetry/src/main/java/org/opensearch/test/telemetry/TestInMemoryHistogram.java +++ b/test/telemetry/src/main/java/org/opensearch/test/telemetry/TestInMemoryHistogram.java @@ -11,7 +11,7 @@ import org.opensearch.telemetry.metrics.Histogram; import org.opensearch.telemetry.metrics.tags.Tags; -import java.util.HashMap; +import java.util.Map; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicInteger; @@ -24,7 +24,7 @@ public class TestInMemoryHistogram implements Histogram { private AtomicInteger histogramValue = new AtomicInteger(0); - private ConcurrentHashMap, Double> histogramValueForTags = new ConcurrentHashMap<>(); + private ConcurrentHashMap, Double> histogramValueForTags = new ConcurrentHashMap<>(); /** * Constructor. @@ -43,7 +43,7 @@ public Integer getHistogramValue() { * Returns the Histogram value for tags * @return */ - public ConcurrentHashMap, Double> getHistogramValueForTags() { + public ConcurrentHashMap, Double> getHistogramValueForTags() { return this.histogramValueForTags; } @@ -54,7 +54,7 @@ public void record(double value) { @Override public synchronized void record(double value, Tags tags) { - HashMap hashMap = (HashMap) tags.getTagsMap(); - histogramValueForTags.put(hashMap, value); + Map tagsMap = tags.getTagsMap(); + histogramValueForTags.put(tagsMap, value); } } From 3a7cda753879dbd3cdb07bf6d435898d4e3e6983 Mon Sep 17 00:00:00 2001 From: Sam Akrah Date: Fri, 6 Mar 2026 16:36:16 -0800 Subject: [PATCH 06/12] retrigger CI Signed-off-by: Sam Akrah From aa323b0a504da251a3d7e0512e6700f0c9660e45 Mon Sep 17 00:00:00 2001 From: Sam Akrah Date: Sat, 7 Mar 2026 19:22:11 -0800 Subject: [PATCH 07/12] retrigger CI Signed-off-by: Sam Akrah From fa2424a9fcfc87e47354134db4363964fc5b579f Mon Sep 17 00:00:00 2001 From: Sam Akrah Date: Sat, 7 Mar 2026 20:38:49 -0800 Subject: [PATCH 08/12] retrigger CI Signed-off-by: Sam Akrah From 078f77f5c71e0878d0ed921690b96d2f0f0e326e Mon Sep 17 00:00:00 2001 From: Sam Akrah Date: Sat, 7 Mar 2026 23:44:48 -0800 Subject: [PATCH 09/12] Remove @Deprecated annotations from experimental Tags API Signed-off-by: Sam Akrah --- .../opensearch/telemetry/metrics/tags/Tags.java | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-) diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java index a8a7b150639c0..663ee24443038 100644 --- a/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java +++ b/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java @@ -291,16 +291,10 @@ public String toString() { return sb.append('}').toString(); } - // ----------------------------------------------------------------------- - // Deprecated API - // ----------------------------------------------------------------------- - /** * Factory method. - * @return tags - * @deprecated Use {@link #EMPTY} instead. + * @return empty tags */ - @Deprecated public static Tags create() { return EMPTY; } @@ -310,9 +304,7 @@ public static Tags create() { * @param key key * @param value value * @return new Tags instance with the added tag - * @deprecated Use {@link #of} or {@link #concat} instead. */ - @Deprecated public Tags addTag(String key, String value) { Objects.requireNonNull(key, "key cannot be null"); Objects.requireNonNull(value, "value cannot be null"); @@ -324,9 +316,7 @@ public Tags addTag(String key, String value) { * @param key key * @param value value * @return new Tags instance with the added tag - * @deprecated Use {@link #of} or {@link #concat} instead. */ - @Deprecated public Tags addTag(String key, long value) { Objects.requireNonNull(key, "key cannot be null"); return Tags.concat(this, Tags.of(key, (Object) value)); @@ -337,9 +327,7 @@ public Tags addTag(String key, long value) { * @param key key * @param value value * @return new Tags instance with the added tag - * @deprecated Use {@link #of} or {@link #concat} instead. */ - @Deprecated public Tags addTag(String key, double value) { Objects.requireNonNull(key, "key cannot be null"); return Tags.concat(this, Tags.of(key, (Object) value)); @@ -350,9 +338,7 @@ public Tags addTag(String key, double value) { * @param key key * @param value value * @return new Tags instance with the added tag - * @deprecated Use {@link #of} or {@link #concat} instead. */ - @Deprecated public Tags addTag(String key, boolean value) { Objects.requireNonNull(key, "key cannot be null"); return Tags.concat(this, Tags.of(key, (Object) value)); From 08b48c142817448e3edb6dc952e9e9b5aafab1a1 Mon Sep 17 00:00:00 2001 From: Sam Akrah Date: Sun, 8 Mar 2026 08:25:18 -0700 Subject: [PATCH 10/12] retrigger CI Signed-off-by: Sam Akrah From ed5425520ab5fa7970977562349acab83e0d7cd4 Mon Sep 17 00:00:00 2001 From: Sam Akrah Date: Sun, 8 Mar 2026 13:50:18 -0700 Subject: [PATCH 11/12] Validate tag value types to String, Long, Double, or Boolean Tag values are now validated at creation time to ensure only the four types supported by telemetry providers (OTel, Micrometer) are accepted. Unsupported types throw IllegalArgumentException immediately rather than failing silently at the provider boundary. Signed-off-by: Sam Akrah --- .../telemetry/metrics/tags/Tags.java | 19 ++++++ .../telemetry/metrics/tags/TagsTests.java | 60 +++++++++++++++++++ 2 files changed, 79 insertions(+) diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java index 663ee24443038..4a4d14d0428af 100644 --- a/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java +++ b/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java @@ -53,6 +53,7 @@ private Tags(String[] keys, Object[] values, int hashCode) { public static Tags of(String key, Object value) { Objects.requireNonNull(key, "key must not be null"); Objects.requireNonNull(value, "value must not be null"); + validateValue(value); String[] k = { key }; Object[] v = { value }; return new Tags(k, v, computeHash(k, v)); @@ -71,6 +72,8 @@ public static Tags of(String k1, Object v1, String k2, Object v2) { 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); int cmp = k1.compareTo(k2); String[] keys; Object[] values; @@ -104,6 +107,9 @@ public static Tags of(String k1, Object v1, String k2, Object v2, String k3, Obj Objects.requireNonNull(v2, "v2 must not be null"); Objects.requireNonNull(k3, "k3 must not be null"); Objects.requireNonNull(v3, "v3 must not be null"); + validateValue(v1); + validateValue(v2); + validateValue(v3); return fromPairs(new String[] { k1, k2, k3 }, new Object[] { v1, v2, v3 }, 3); } @@ -128,6 +134,10 @@ public static Tags of(String k1, Object v1, String k2, Object v2, String k3, Obj Objects.requireNonNull(v3, "v3 must not be null"); Objects.requireNonNull(k4, "k4 must not be null"); Objects.requireNonNull(v4, "v4 must not be null"); + validateValue(v1); + validateValue(v2); + validateValue(v3); + validateValue(v4); return fromPairs(new String[] { k1, k2, k3, k4 }, new Object[] { v1, v2, v3, v4 }, 4); } @@ -215,6 +225,7 @@ public static Tags fromMap(Map map) { 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)); } @@ -348,6 +359,14 @@ public Tags addTag(String key, boolean value) { // Internal // ----------------------------------------------------------------------- + private static final String UNSUPPORTED_TYPE_MSG = "Tag value must be String, Long, Double, or Boolean, got: "; + + private static void validateValue(Object value) { + if (!(value instanceof String || value instanceof Long || value instanceof Double || value instanceof Boolean)) { + throw new IllegalArgumentException(UNSUPPORTED_TYPE_MSG + value.getClass().getName()); + } + } + private static int computeHash(String[] keys, Object[] values) { int result = 1; for (int i = 0; i < keys.length; i++) { diff --git a/libs/telemetry/src/test/java/org/opensearch/telemetry/metrics/tags/TagsTests.java b/libs/telemetry/src/test/java/org/opensearch/telemetry/metrics/tags/TagsTests.java index 83dbf3fc3bce6..38cfd48c550e7 100644 --- a/libs/telemetry/src/test/java/org/opensearch/telemetry/metrics/tags/TagsTests.java +++ b/libs/telemetry/src/test/java/org/opensearch/telemetry/metrics/tags/TagsTests.java @@ -10,6 +10,7 @@ import org.opensearch.test.OpenSearchTestCase; +import java.util.ArrayList; import java.util.HashMap; import java.util.Map; @@ -73,6 +74,10 @@ public void testOfStringPairsEmpty() { assertSame(Tags.EMPTY, Tags.ofStringPairs(new String[0])); } + public void testOfStringPairsNull() { + assertSame(Tags.EMPTY, Tags.ofStringPairs((String[]) null)); + } + public void testOfStringPairsOddLengthThrows() { expectThrows(IllegalArgumentException.class, () -> Tags.ofStringPairs("a", "b", "c")); } @@ -120,6 +125,10 @@ public void testConcatBothNullReturnsEmpty() { assertSame(Tags.EMPTY, Tags.concat(null, null)); } + public void testConcatEmptyWithNullReturnsEmpty() { + assertSame(Tags.EMPTY, Tags.concat(Tags.EMPTY, null)); + } + public void testConcatPartialOverlapMergesAndDeduplicates() { Tags a = Tags.of("a", "1", "c", "3"); Tags b = Tags.of("b", "2", "c", "4"); @@ -209,6 +218,49 @@ public void testFromMapNullValueThrows() { expectThrows(NullPointerException.class, () -> Tags.fromMap(map)); } + // --- value type validation --- + + public void testOfAcceptsSupportedTypes() { + Tags stringTag = Tags.of("s", "val"); + assertEquals("val", stringTag.getValue(0)); + + Tags longTag = Tags.of("l", 42L); + assertEquals(42L, longTag.getValue(0)); + + Tags doubleTag = Tags.of("d", 3.14); + assertEquals(3.14, doubleTag.getValue(0)); + + Tags boolTag = Tags.of("b", true); + assertEquals(true, boolTag.getValue(0)); + } + + public void testOfRejectsUnsupportedType() { + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> Tags.of("k", new ArrayList<>())); + assertTrue(e.getMessage().contains("ArrayList")); + } + + public void testOfRejectsIntegerType() { + expectThrows(IllegalArgumentException.class, () -> Tags.of("k", 42)); + } + + public void testOfTwoPairRejectsUnsupportedType() { + expectThrows(IllegalArgumentException.class, () -> Tags.of("a", "ok", "b", new Object())); + } + + public void testOfThreePairRejectsUnsupportedType() { + expectThrows(IllegalArgumentException.class, () -> Tags.of("a", "ok", "b", "ok", "c", new int[] { 1 })); + } + + public void testOfFourPairRejectsUnsupportedType() { + expectThrows(IllegalArgumentException.class, () -> Tags.of("a", "ok", "b", "ok", "c", "ok", "d", new Object())); + } + + public void testFromMapRejectsUnsupportedValueType() { + Map map = new HashMap<>(); + map.put("k", new ArrayList<>()); + expectThrows(IllegalArgumentException.class, () -> Tags.fromMap(map)); + } + // --- getTagsMap --- public void testGetTagsMapPreservesOriginalTypes() { @@ -240,6 +292,14 @@ public void testIdentityEquals() { assertEquals(t, t); } + public void testNotEqualToNull() { + assertNotEquals(Tags.of("k", "v"), null); + } + + public void testNotEqualToNonTagsObject() { + assertNotEquals(Tags.of("k", "v"), "not a Tags"); + } + public void testDifferentSizeNotEqual() { Tags a = Tags.of("k", "v"); Tags b = Tags.of("k", "v", "k2", "v2"); From 47bebbd217c92c74686bf015b7e1058a6b6b5db4 Mon Sep 17 00:00:00 2001 From: Sam Akrah Date: Mon, 9 Mar 2026 10:13:28 -0700 Subject: [PATCH 12/12] Added method overloads Signed-off-by: Sam Akrah --- .../telemetry/metrics/tags/Tags.java | 113 +++++------------- .../telemetry/metrics/tags/TagsTests.java | 106 +++++----------- 2 files changed, 58 insertions(+), 161 deletions(-) diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java index 4a4d14d0428af..8f7311cecad83 100644 --- a/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java +++ b/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/tags/Tags.java @@ -45,100 +45,56 @@ private Tags(String[] keys, Object[] values, int hashCode) { // ----------------------------------------------------------------------- /** - * Creates an immutable Tags with one key-value pair. + * Creates an immutable Tags with one String-valued pair. * @param key tag key * @param value tag value * @return new Tags instance */ - public static Tags of(String key, Object value) { + public static Tags of(String key, String value) { Objects.requireNonNull(key, "key must not be null"); Objects.requireNonNull(value, "value must not be null"); - validateValue(value); String[] k = { key }; Object[] v = { value }; return new Tags(k, v, computeHash(k, v)); } /** - * Creates an immutable Tags with two key-value pairs. - * @param k1 first key - * @param v1 first value - * @param k2 second key - * @param v2 second value + * Creates an immutable Tags with one long-valued pair. + * @param key tag key + * @param value tag value * @return new Tags instance */ - public static Tags of(String k1, Object v1, String k2, Object v2) { - 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); - 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)); + public static Tags of(String key, long value) { + Objects.requireNonNull(key, "key must not be null"); + String[] k = { key }; + Object[] v = { value }; + return new Tags(k, v, computeHash(k, v)); } /** - * Creates an immutable Tags with three key-value pairs. - * @param k1 first key - * @param v1 first value - * @param k2 second key - * @param v2 second value - * @param k3 third key - * @param v3 third value + * Creates an immutable Tags with one double-valued pair. + * @param key tag key + * @param value tag value * @return new Tags instance */ - public static Tags of(String k1, Object v1, String k2, Object v2, String k3, Object v3) { - 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"); - Objects.requireNonNull(k3, "k3 must not be null"); - Objects.requireNonNull(v3, "v3 must not be null"); - validateValue(v1); - validateValue(v2); - validateValue(v3); - return fromPairs(new String[] { k1, k2, k3 }, new Object[] { v1, v2, v3 }, 3); + public static Tags of(String key, double value) { + Objects.requireNonNull(key, "key must not be null"); + String[] k = { key }; + Object[] v = { value }; + return new Tags(k, v, computeHash(k, v)); } /** - * Creates an immutable Tags with four key-value pairs. - * @param k1 first key - * @param v1 first value - * @param k2 second key - * @param v2 second value - * @param k3 third key - * @param v3 third value - * @param k4 fourth key - * @param v4 fourth value + * Creates an immutable Tags with one boolean-valued pair. + * @param key tag key + * @param value tag value * @return new Tags instance */ - public static Tags of(String k1, Object v1, String k2, Object v2, String k3, Object v3, String k4, Object v4) { - 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"); - Objects.requireNonNull(k3, "k3 must not be null"); - Objects.requireNonNull(v3, "v3 must not be null"); - Objects.requireNonNull(k4, "k4 must not be null"); - Objects.requireNonNull(v4, "v4 must not be null"); - validateValue(v1); - validateValue(v2); - validateValue(v3); - validateValue(v4); - return fromPairs(new String[] { k1, k2, k3, k4 }, new Object[] { v1, v2, v3, v4 }, 4); + public static Tags of(String key, boolean value) { + Objects.requireNonNull(key, "key must not be null"); + String[] k = { key }; + Object[] v = { value }; + return new Tags(k, v, computeHash(k, v)); } /** @@ -225,7 +181,6 @@ public static Tags fromMap(Map map) { 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)); } @@ -319,7 +274,7 @@ public static Tags create() { public Tags addTag(String key, String value) { Objects.requireNonNull(key, "key cannot be null"); Objects.requireNonNull(value, "value cannot be null"); - return Tags.concat(this, Tags.of(key, (Object) value)); + return Tags.concat(this, Tags.of(key, value)); } /** @@ -330,7 +285,7 @@ public Tags addTag(String key, String value) { */ public Tags addTag(String key, long value) { Objects.requireNonNull(key, "key cannot be null"); - return Tags.concat(this, Tags.of(key, (Object) value)); + return Tags.concat(this, Tags.of(key, value)); } /** @@ -341,7 +296,7 @@ public Tags addTag(String key, long value) { */ public Tags addTag(String key, double value) { Objects.requireNonNull(key, "key cannot be null"); - return Tags.concat(this, Tags.of(key, (Object) value)); + return Tags.concat(this, Tags.of(key, value)); } /** @@ -352,21 +307,13 @@ public Tags addTag(String key, double value) { */ public Tags addTag(String key, boolean value) { Objects.requireNonNull(key, "key cannot be null"); - return Tags.concat(this, Tags.of(key, (Object) value)); + return Tags.concat(this, Tags.of(key, value)); } // ----------------------------------------------------------------------- // Internal // ----------------------------------------------------------------------- - private static final String UNSUPPORTED_TYPE_MSG = "Tag value must be String, Long, Double, or Boolean, got: "; - - private static void validateValue(Object value) { - if (!(value instanceof String || value instanceof Long || value instanceof Double || value instanceof Boolean)) { - throw new IllegalArgumentException(UNSUPPORTED_TYPE_MSG + value.getClass().getName()); - } - } - private static int computeHash(String[] keys, Object[] values) { int result = 1; for (int i = 0; i < keys.length; i++) { diff --git a/libs/telemetry/src/test/java/org/opensearch/telemetry/metrics/tags/TagsTests.java b/libs/telemetry/src/test/java/org/opensearch/telemetry/metrics/tags/TagsTests.java index 38cfd48c550e7..c8eb4be01efc7 100644 --- a/libs/telemetry/src/test/java/org/opensearch/telemetry/metrics/tags/TagsTests.java +++ b/libs/telemetry/src/test/java/org/opensearch/telemetry/metrics/tags/TagsTests.java @@ -10,7 +10,6 @@ import org.opensearch.test.OpenSearchTestCase; -import java.util.ArrayList; import java.util.HashMap; import java.util.Map; @@ -40,32 +39,26 @@ public void testOfNullKeyThrows() { } public void testOfNullValueThrows() { - expectThrows(NullPointerException.class, () -> Tags.of("k", (Object) null)); + expectThrows(NullPointerException.class, () -> Tags.of("k", (String) null)); } - // --- of(k1,v1,k2,v2) sorted --- - - public void testOfTwoTagsSorted() { - Tags t = Tags.of("z", "1", "a", "2"); - assertEquals(2, t.size()); - assertEquals("a", t.getKey(0)); - assertEquals("z", t.getKey(1)); + public void testOfLong() { + Tags t = Tags.of("retries", 3L); + assertEquals(1, t.size()); + assertEquals("retries", t.getKey(0)); + assertEquals(3L, t.getValue(0)); } - public void testOfTwoTagsDuplicateKeyLastWins() { - Tags t = Tags.of("k", "first", "k", "second"); + public void testOfDouble() { + Tags t = Tags.of("latency", 1.5); assertEquals(1, t.size()); - assertEquals("second", t.getValue(0)); + assertEquals(1.5, t.getValue(0)); } - // --- of(k1,v1,k2,v2,k3,v3) --- - - public void testOfThreeTagsSorted() { - Tags t = Tags.of("c", "3", "a", "1", "b", "2"); - assertEquals(3, t.size()); - assertEquals("a", t.getKey(0)); - assertEquals("b", t.getKey(1)); - assertEquals("c", t.getKey(2)); + public void testOfBoolean() { + Tags t = Tags.of("enabled", true); + assertEquals(1, t.size()); + assertEquals(true, t.getValue(0)); } // --- ofStringPairs(varargs) --- @@ -130,8 +123,8 @@ public void testConcatEmptyWithNullReturnsEmpty() { } public void testConcatPartialOverlapMergesAndDeduplicates() { - Tags a = Tags.of("a", "1", "c", "3"); - Tags b = Tags.of("b", "2", "c", "4"); + Tags a = Tags.ofStringPairs("a", "1", "c", "3"); + Tags b = Tags.ofStringPairs("b", "2", "c", "4"); Tags merged = Tags.concat(a, b); assertEquals(3, merged.size()); assertEquals("a", merged.getKey(0)); @@ -143,8 +136,8 @@ public void testConcatPartialOverlapMergesAndDeduplicates() { } public void testConcatInterleavedNoOverlap() { - Tags a = Tags.of("a", "1", "c", "3", "e", "5"); - Tags b = Tags.of("b", "2", "d", "4"); + Tags a = Tags.ofStringPairs("a", "1", "c", "3", "e", "5"); + Tags b = Tags.ofStringPairs("b", "2", "d", "4"); Tags merged = Tags.concat(a, b); assertEquals(5, merged.size()); assertEquals("a", merged.getKey(0)); @@ -155,8 +148,8 @@ public void testConcatInterleavedNoOverlap() { } public void testConcatFullOverlapBWins() { - Tags a = Tags.of("a", "old_a", "b", "old_b"); - Tags b = Tags.of("a", "new_a", "b", "new_b"); + Tags a = Tags.ofStringPairs("a", "old_a", "b", "old_b"); + Tags b = Tags.ofStringPairs("a", "new_a", "b", "new_b"); Tags merged = Tags.concat(a, b); assertEquals(2, merged.size()); assertEquals("new_a", merged.getValue(0)); @@ -165,7 +158,7 @@ public void testConcatFullOverlapBWins() { public void testConcatLargeRemainderPath() { Tags a = Tags.of("z", "26"); - Tags b = Tags.of("a", "1", "b", "2", "c", "3", "d", "4"); + Tags b = Tags.ofStringPairs("a", "1", "b", "2", "c", "3", "d", "4"); Tags merged = Tags.concat(a, b); assertEquals(5, merged.size()); assertEquals("a", merged.getKey(0)); @@ -175,15 +168,15 @@ public void testConcatLargeRemainderPath() { } public void testConcatHashConsistency() { - Tags viaOf = Tags.of("a", "1", "b", "2"); + Tags viaOfStringPairs = Tags.ofStringPairs("a", "1", "b", "2"); Tags viaConcat = Tags.concat(Tags.of("a", "1"), Tags.of("b", "2")); - assertEquals(viaOf, viaConcat); - assertEquals(viaOf.hashCode(), viaConcat.hashCode()); + assertEquals(viaOfStringPairs, viaConcat); + assertEquals(viaOfStringPairs.hashCode(), viaConcat.hashCode()); } public void testConcatResultIsSorted() { Tags a = Tags.of("x", "1"); - Tags b = Tags.of("a", "2", "m", "3"); + Tags b = Tags.ofStringPairs("a", "2", "m", "3"); Tags merged = Tags.concat(a, b); for (int i = 0; i < merged.size() - 1; i++) { assertTrue(merged.getKey(i).compareTo(merged.getKey(i + 1)) < 0); @@ -218,61 +211,18 @@ public void testFromMapNullValueThrows() { expectThrows(NullPointerException.class, () -> Tags.fromMap(map)); } - // --- value type validation --- - - public void testOfAcceptsSupportedTypes() { - Tags stringTag = Tags.of("s", "val"); - assertEquals("val", stringTag.getValue(0)); - - Tags longTag = Tags.of("l", 42L); - assertEquals(42L, longTag.getValue(0)); - - Tags doubleTag = Tags.of("d", 3.14); - assertEquals(3.14, doubleTag.getValue(0)); - - Tags boolTag = Tags.of("b", true); - assertEquals(true, boolTag.getValue(0)); - } - - public void testOfRejectsUnsupportedType() { - IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> Tags.of("k", new ArrayList<>())); - assertTrue(e.getMessage().contains("ArrayList")); - } - - public void testOfRejectsIntegerType() { - expectThrows(IllegalArgumentException.class, () -> Tags.of("k", 42)); - } - - public void testOfTwoPairRejectsUnsupportedType() { - expectThrows(IllegalArgumentException.class, () -> Tags.of("a", "ok", "b", new Object())); - } - - public void testOfThreePairRejectsUnsupportedType() { - expectThrows(IllegalArgumentException.class, () -> Tags.of("a", "ok", "b", "ok", "c", new int[] { 1 })); - } - - public void testOfFourPairRejectsUnsupportedType() { - expectThrows(IllegalArgumentException.class, () -> Tags.of("a", "ok", "b", "ok", "c", "ok", "d", new Object())); - } - - public void testFromMapRejectsUnsupportedValueType() { - Map map = new HashMap<>(); - map.put("k", new ArrayList<>()); - expectThrows(IllegalArgumentException.class, () -> Tags.fromMap(map)); - } - // --- getTagsMap --- public void testGetTagsMapPreservesOriginalTypes() { - Tags t = Tags.of("num", (Object) 42L); + Tags t = Tags.of("num", 42L); assertEquals(42L, t.getTagsMap().get("num")); } // --- equals / hashCode --- public void testEqualTagsAreEqual() { - Tags a = Tags.of("x", "1", "y", "2"); - Tags b = Tags.of("x", "1", "y", "2"); + Tags a = Tags.ofStringPairs("x", "1", "y", "2"); + Tags b = Tags.ofStringPairs("x", "1", "y", "2"); assertEquals(a, b); assertEquals(a.hashCode(), b.hashCode()); } @@ -302,7 +252,7 @@ public void testNotEqualToNonTagsObject() { public void testDifferentSizeNotEqual() { Tags a = Tags.of("k", "v"); - Tags b = Tags.of("k", "v", "k2", "v2"); + Tags b = Tags.ofStringPairs("k", "v", "k2", "v2"); assertNotEquals(a, b); }