Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add configurable commitAllMetricUpdates #124

Merged

Conversation

FuRyanf
Copy link

@FuRyanf FuRyanf commented Aug 30, 2024

Description of Changes

This update introduces a configurable option to control the invocation of commitUpdates within the updateMetrics method in the SamzaMetricsContainer class. The new configuration option, beam.samza.metrics.commitAllMetricUpdates, allows toggling the behavior of committing metrics after they have been processed.

Changes:

  • New Configuration:

    • Added a new configuration property, beam.samza.metrics.commitAllMetricUpdates, which determines whether commitUpdates is called for each metrics container after metrics are updated.
  • Code Logic:

    • The line stepNameList.stream().map(metricsContainers::getContainer).forEach(MetricsContainerImpl::commitUpdates); ensures that metrics are marked as committed, preventing them from being reprocessed in subsequent steps. This is particularly important for avoiding the accumulation of metrics and the associated CPU overhead.

Rationale:

  • Performance Considerations:

    • The commitUpdates method, similar to getUpdates, may have a significant CPU cost. Preliminary analysis shows that getUpdates contributed 8% of CPU time for the updateMetrics call. Given that commitUpdates could incur similar overhead, the configuration option provides flexibility to disable this behavior for jobs where all metrics in a step are always used, making the commit operation unnecessary.
  • Testing and Flexibility:

    • By making the commit behavior configurable, we can test the impact of this change on various jobs and optimize performance accordingly. This allows us to determine if the additional CPU cost of committing updates is justified based on the specific needs of different workloads.

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)

Build python source distribution and wheels
Python tests
Java tests
Go tests

See CI.md for more information about GitHub Actions CI.

@FuRyanf FuRyanf force-pushed the FuRyanf/commit-metric-updates branch 3 times, most recently from 12bb967 to 0a54c22 Compare August 30, 2024 18:56
@FuRyanf FuRyanf closed this Sep 3, 2024
@FuRyanf FuRyanf reopened this Sep 3, 2024
@github-actions github-actions bot added the build label Sep 3, 2024
@FuRyanf FuRyanf force-pushed the FuRyanf/commit-metric-updates branch from 56fe78f to fd64e7a Compare September 3, 2024 19:39
@FuRyanf FuRyanf force-pushed the FuRyanf/commit-metric-updates branch from fd64e7a to 67eb2c2 Compare September 3, 2024 21:11
@github-actions github-actions bot added the java label Sep 3, 2024
@github-actions github-actions bot removed the java label Sep 3, 2024
@xinyuiscool xinyuiscool merged commit fa75dd3 into linkedin:li_trunk Sep 4, 2024
6 checks passed
@FuRyanf FuRyanf deleted the FuRyanf/commit-metric-updates branch September 13, 2024 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants