Add support for StringSet metric in Java SDK to track set of unique s… #31789

rohitsinha54 · 2024-07-06T01:23:45Z

Add support for StringSet metric in Java SDK to track set of unique string as metric.
addresses #31788

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.

…tring as metric.

rohitsinha54 · 2024-07-06T01:25:00Z

R: @robertwb

github-actions · 2024-07-06T01:26:12Z

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control

…a checks

robertwb

Thanks, this looks realy good.

robertwb · 2024-07-07T05:48:58Z

runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/MetricsContainerImpl.java

+  }
+
+  /**
+   * Return a {@code StringSetCell} named {@code metricName}.If it doesn't exist, return {@code


Nit: space after period.

robertwb · 2024-07-07T05:51:33Z

...rs/core-java/src/main/java/org/apache/beam/runners/core/metrics/MonitoringInfoEncodings.java

+  public static ByteString encodeStringSet(StringSetData data) {
+    try (ByteStringOutputStream output = new ByteStringOutputStream()) {
+      // encode the length of set
+      STRING_CODER.encode(String.valueOf(data.stringSet().size()), output);


Use IterableCoder.of(StringsUtf8Coder.of()) rather than encoding the length as a string.

This is nice!!! Thank you.

(I will work on this and other review comments tomorrow. Didn't realize the difference between "Add single comment" and "start review" in previous comment)

robertwb · 2024-07-07T05:55:05Z

runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/StringSetCell.java

+
+  @Override
+  public void add(String value) {
+    update(StringSetData.create(new HashSet<>(Collections.singletonList(value))));


Perhaps optimized for the case where value is already in the set by skipping the update (and creation of sets and just to do a no-op merge, and setting the dirty bit).

Good idea. Thanks.

robertwb · 2024-07-07T05:57:39Z

runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/StringSetData.java

+@AutoValue
+public abstract class StringSetData implements Serializable {
+
+  public abstract Set<String> stringSet();


Perhaps here and below we should document whether the sets are (expected to be?) immutable or not. E.g. is one allowed ot modify the set after passing it to create? Modify the set returned from stringSet()?

Done.
Also I see I missed to set well defined mutability behavior for StringSetData and Result. Fixed it now. Result needs to immutable. Data I think we can have either ways, having mutable will allow being able combine more efficiently but can lead to confusing contract specially for EmptyStringSetData. I am going to make StringSetData immutable and they can be combined by only copying

Note: We ended up changing StringSetData to be mutable because in certain IOs such as TextIOs the number lineage records (per file) are very large and immutable hashset copy cost became huge

https://github.com/apache/beam/pull/32650/files#diff-5bbbc1e2641f65bfb9040d37f09cddb696d642e7a0bb5f4779b996350141b9ba

robertwb · 2024-07-07T05:59:37Z

runners/core-java/src/test/java/org/apache/beam/runners/core/metrics/StringSetCellTest.java

+
+    StringSetCell differentDirty = new StringSetCell(MetricName.named("namespace", "name"));
+    differentDirty.getDirty().afterModification();
+    Assert.assertNotEquals(stringSetCell, differentDirty);


I'll admit this feels a bit odd. But when do cells need to be compared (or be hashable)? (If it's just following the convention of what's done elsewhere, that's fine.)

This is just conventional. I did not find any direct (or even indirect) usage in my work so far.

robertwb · 2024-07-07T06:02:39Z

...ogle-cloud-dataflow-java/src/main/java/org/apache/beam/runners/dataflow/DataflowMetrics.java

+      if (metricUpdate.getSet() == null) {
+        return StringSetResult.empty();
+      }
+      return StringSetResult.create(new HashSet<>(((ArrayList) metricUpdate.getSet())));


ImmutableSet.copyOf(...) is likely faster, especially for sets of {0,1} elements. (Might be worth using elsewhere too.)

Oh ya true. Thanks.

robertwb · 2024-07-07T06:03:55Z

...-cloud-dataflow-java/src/test/java/org/apache/beam/runners/dataflow/DataflowMetricsTest.java

@@ -184,6 +189,13 @@ private MetricUpdate makeCounterMetricUpdate(
    return setStructuredName(update, name, namespace, step, tentative);
  }

+  private MetricUpdate makeStringSetMetricUpdate(
+      String name, String namespace, String step, List<String> setValues, boolean tentative) {


Why is this a List of values? Should it be a Set or Container?

Ah you are right it can be set. Fixed. Earlier the documentation tripped me https://screenshot.googleplex.com/4npsge3GVUo3zRs I thought it needs to be "list" .

robertwb · 2024-07-07T06:07:08Z

runners/jet/src/main/java/org/apache/beam/runners/jet/metrics/StringSetImpl.java

+
+  @Override
+  public void add(String value) {
+    stringSetData.combine(StringSetData.create(ImmutableSet.of("ab")));


Again, perhaps worth optimizing for the case where value is already present.

Fixed. Also fixed my typo of ignoring value and using "ab".

robertwb · 2024-07-07T06:11:34Z

runners/direct-java/src/main/java/org/apache/beam/runners/direct/DirectMetrics.java

+        public StringSetData combine(Iterable<StringSetData> updates) {
+          StringSetData result = StringSetData.empty();
+          for (StringSetData update : updates) {
+            result = result.combine(update);


Doing a one-at-a-time binary combine of multiple sets might be inefficient (vs. creating a single set and adding everything to it). I don't know how often this method is called thoug.

Ah good point. Done.

rohitsinha54

@robertwb
Thanks for the prompt review. Fixed review comment. Please have another look.

Thank you.

rohitsinha54 · 2024-07-07T16:36:41Z

runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/StringSetCell.java

+
+  @Override
+  public void add(String value) {
+    update(StringSetData.create(new HashSet<>(Collections.singletonList(value))));


Good idea. Thanks.

rohitsinha54 · 2024-07-07T17:17:13Z

runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/StringSetData.java

+@AutoValue
+public abstract class StringSetData implements Serializable {
+
+  public abstract Set<String> stringSet();


Done.
Also I see I missed to set well defined mutability behavior for StringSetData and Result. Fixed it now. Result needs to immutable. Data I think we can have either ways, having mutable will allow being able combine more efficiently but can lead to confusing contract specially for EmptyStringSetData. I am going to make StringSetData immutable and they can be combined by only copying

rohitsinha54 · 2024-07-07T19:30:40Z

runners/core-java/src/test/java/org/apache/beam/runners/core/metrics/StringSetCellTest.java

+
+    StringSetCell differentDirty = new StringSetCell(MetricName.named("namespace", "name"));
+    differentDirty.getDirty().afterModification();
+    Assert.assertNotEquals(stringSetCell, differentDirty);


This is just conventional. I did not find any direct (or even indirect) usage in my work so far.

rohitsinha54 · 2024-07-07T20:21:50Z

runners/direct-java/src/main/java/org/apache/beam/runners/direct/DirectMetrics.java

+        public StringSetData combine(Iterable<StringSetData> updates) {
+          StringSetData result = StringSetData.empty();
+          for (StringSetData update : updates) {
+            result = result.combine(update);


Ah good point. Done.

rohitsinha54 · 2024-07-07T20:27:00Z

...ogle-cloud-dataflow-java/src/main/java/org/apache/beam/runners/dataflow/DataflowMetrics.java

+      if (metricUpdate.getSet() == null) {
+        return StringSetResult.empty();
+      }
+      return StringSetResult.create(new HashSet<>(((ArrayList) metricUpdate.getSet())));


Oh ya true. Thanks.

rohitsinha54 · 2024-07-07T20:43:03Z

runners/jet/src/main/java/org/apache/beam/runners/jet/metrics/StringSetImpl.java

+
+  @Override
+  public void add(String value) {
+    stringSetData.combine(StringSetData.create(ImmutableSet.of("ab")));


Fixed. Also fixed my typo of ignoring value and using "ab".

rohitsinha54 · 2024-07-08T02:59:22Z

...-cloud-dataflow-java/src/test/java/org/apache/beam/runners/dataflow/DataflowMetricsTest.java

@@ -184,6 +189,13 @@ private MetricUpdate makeCounterMetricUpdate(
    return setStructuredName(update, name, namespace, step, tentative);
  }

+  private MetricUpdate makeStringSetMetricUpdate(
+      String name, String namespace, String step, List<String> setValues, boolean tentative) {


Ah you are right it can be set. Fixed. Earlier the documentation tripped me https://screenshot.googleplex.com/4npsge3GVUo3zRs I thought it needs to be "list" .

* Null-containing sets don't need to be tested as they can no longer be constructed. * Use vendered guava.

Add support for StringSet metric in Java SDK to track set of unique string as metric. addresses apache#31788 * Add support for StringSet metric in Java SDK to track set of unique string as metric. * Fix compilation and tests * Add support for StringSet in PortableRunner and fix some spotless java checks * Add support for StringSet in JetRunner * Fix precommit errors * Fixes for review comments * Other fixes * Fixes for spotless java * Fix a couple of tests. * Null-containing sets don't need to be tested as they can no longer be constructed. * Use vendered guava. * unused imports --------- Co-authored-by: Robert Bradshaw <[email protected]>

Add support for StringSet metric in Java SDK to track set of unique s…

b733f96

…tring as metric.

github-actions bot added java runners dataflow direct core labels Jul 6, 2024

github-actions bot added extensions jet portability labels Jul 6, 2024

rohitsinha54 force-pushed the stringset-java branch from 04d542f to 35ccdb1 Compare July 6, 2024 06:25

Fix compilation and tests

44a3901

rohitsinha54 force-pushed the stringset-java branch from 35ccdb1 to 44a3901 Compare July 6, 2024 15:50

Add support for StringSet in PortableRunner and fix some spotless jav…

e16da38

…a checks

github-actions bot added the model label Jul 6, 2024

rohitsinha54 added 2 commits July 6, 2024 14:07

Add support for StringSet in JetRunner

2dd784b

Fix precommit errors

67ef5bd

rohitsinha54 force-pushed the stringset-java branch from ecfa14e to 67ef5bd Compare July 6, 2024 23:57

robertwb reviewed Jul 7, 2024

View reviewed changes

Fixes for review comments

0ca3bff

rohitsinha54 force-pushed the stringset-java branch from ad17a69 to 0ca3bff Compare July 8, 2024 04:53

rohitsinha54 added 2 commits July 7, 2024 22:54

Other fixes

ef55463

Fixes for spotless java

e43e691

rohitsinha54 commented Jul 8, 2024

View reviewed changes

robertwb added 2 commits July 8, 2024 13:06

Fix a couple of tests.

6f15be1

* Null-containing sets don't need to be tested as they can no longer be constructed. * Use vendered guava.

unused imports

adcf3df

robertwb merged commit de4645d into apache:master Jul 8, 2024
113 checks passed

Abacn mentioned this pull request Jul 9, 2024

[Failing Test]: ValidatesRunner tests failing due to StringSet counter #31814

Closed

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for StringSet metric in Java SDK to track set of unique s… #31789

Add support for StringSet metric in Java SDK to track set of unique s… #31789

rohitsinha54 commented Jul 6, 2024

rohitsinha54 commented Jul 6, 2024

github-actions bot commented Jul 6, 2024

robertwb left a comment

robertwb Jul 7, 2024

rohitsinha54 Jul 7, 2024

robertwb Jul 7, 2024

rohitsinha54 Jul 7, 2024

robertwb Jul 7, 2024

rohitsinha54 Jul 7, 2024

robertwb Jul 7, 2024

rohitsinha54 Jul 7, 2024

rohitsinha54 Oct 18, 2024

robertwb Jul 7, 2024

rohitsinha54 Jul 7, 2024

robertwb Jul 7, 2024

rohitsinha54 Jul 7, 2024

robertwb Jul 7, 2024

rohitsinha54 Jul 8, 2024

robertwb Jul 7, 2024

rohitsinha54 Jul 7, 2024

robertwb Jul 7, 2024

rohitsinha54 Jul 7, 2024

rohitsinha54 left a comment

rohitsinha54 Jul 7, 2024

rohitsinha54 Jul 7, 2024

rohitsinha54 Jul 7, 2024

rohitsinha54 Jul 7, 2024

rohitsinha54 Jul 7, 2024

rohitsinha54 Jul 7, 2024

rohitsinha54 Jul 8, 2024

Add support for StringSet metric in Java SDK to track set of unique s… #31789

Add support for StringSet metric in Java SDK to track set of unique s… #31789

Conversation

rohitsinha54 commented Jul 6, 2024

GitHub Actions Tests Status (on master branch)

rohitsinha54 commented Jul 6, 2024

github-actions bot commented Jul 6, 2024

robertwb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rohitsinha54 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment