Introduce pipeline options to disable user counter and user stringset #33059
Conversation
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
| */ | ||
| @Internal | ||
| public static void setDefaultPipelineOptions(PipelineOptions options) { | ||
| checkNotNull(options, "options cannot be null"); | ||
| long id = options.getOptionsId(); | ||
| int nextRevision = options.revision(); | ||
|
|
||
| // entry to set other PipelineOption determined flags | ||
| Metrics.setDefaultPipelineOptions(options); |
There was a problem hiding this comment.
This makes this a little overloaded no? seems odd we set metric stuff in FS. But I see you want to do this in centralized place.
Would it better to have it separate in DataflowBatchWorkerHarness.java, StreamingDataflowWorker.java and FnHarness.java from where all this is called?
(I do not have a preference, just a though. Feel free to ignore this comment if you want to do it this way)
There was a problem hiding this comment.
setDefaultPipelineOptions will be called once per worker, on SDK harness startup, so the overloead is minimum
Separate call has much larger diff. Multiple runners has their own main() function (search for FileSystems.setDefaultPipelineOptions) give a hint
| if (counterDisabled) { | ||
| LOG.info("Counter metrics are disabled."); | ||
| } | ||
| boolean stringSetDisabled = ExperimentalOptions.hasExperiment(exp, "disableStringSetMetrics"); |
There was a problem hiding this comment.
With this if a streaming customer want to disable StringSet or Lineage in their job they will need to restart the job passing this experiment.
On service side we have an experiment to enable lineage (by default disabled). It is already present in pipeline option https://screenshot.googleplex.com/7WaPSvq85nHyCEt I was thinking we should keep lineage metric reporting disabled by default unless a job enable lineage.
We have identified some cases where for long running streaming jobs the lineage information can get very large over time so it will be ideal to only capture this for job which opt for lineage.
Can I please ask you while you are doing the same thing in this PR to add a
Lineage.setDefaultPipelineOptions(options);
in file above
and this similar function in Lineage class to set based on the lineage experiment above enable_lineage and the add in Lineage class just return as no-op if lineage is not enabled
Thank you in advance.
There was a problem hiding this comment.
Thanks, closed this one and opened an alternative PR: #33085 for this suggestion.
| @@ -174,6 +231,9 @@ private DelegatingStringSet(MetricName name) { | |||
|
|
|||
| @Override | |||
| public void add(String value) { | |||
| if (MetricsFlag.stringSetDisabled()) { | |||
There was a problem hiding this comment.
Should this part have unit test
|
After discussion choose to proceed with this approach. IOs PreCommit testReadWithMetadata flaky test not related to this PR |
|
testTeardownCalledAfterExceptionInProcessElementStateful (org.apache.beam.sdk.transforms.ParDoLifecycleTest) failed, not related to this PR, merging for now |
Fix #33055
Please add a meaningful description for your change here
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
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, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.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.