-
Notifications
You must be signed in to change notification settings - Fork 6
Adding Percentiles to Beam Distribution Metric #16
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
Adding Percentiles to Beam Distribution Metric #16
Conversation
a05daa3 to
fb32e60
Compare
runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/DistributionData.java
Outdated
Show resolved
Hide resolved
sdks/java/core/src/test/java/org/apache/beam/sdk/metrics/MetricsTest.java
Outdated
Show resolved
Hide resolved
fb32e60 to
cd0e692
Compare
runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/DistributionData.java
Outdated
Show resolved
Hide resolved
runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/DistributionData.java
Outdated
Show resolved
Hide resolved
runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/DistributionData.java
Outdated
Show resolved
Hide resolved
runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/DistributionData.java
Outdated
Show resolved
Hide resolved
e0061de to
c85fe00
Compare
xinyuiscool
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks fore adding this metric!
3ae64af to
6d8c00f
Compare
runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/DistributionData.java
Outdated
Show resolved
Hide resolved
runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/DistributionData.java
Outdated
Show resolved
Hide resolved
runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/DistributionData.java
Outdated
Show resolved
Hide resolved
runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/MetricsContainerImpl.java
Outdated
Show resolved
Hide resolved
sdks/java/core/src/main/java/org/apache/beam/sdk/metrics/Metrics.java
Outdated
Show resolved
Hide resolved
sdks/java/core/src/main/java/org/apache/beam/sdk/metrics/Metrics.java
Outdated
Show resolved
Hide resolved
runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/DistributionCell.java
Outdated
Show resolved
Hide resolved
runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/DistributionData.java
Outdated
Show resolved
Hide resolved
sdks/java/core/src/main/java/org/apache/beam/sdk/metrics/Metrics.java
Outdated
Show resolved
Hide resolved
kw2542
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
f4be093 to
3dcc7af
Compare
sdks/java/core/src/main/java/org/apache/beam/sdk/metrics/Metrics.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do the empty check here.
runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/DistributionData.java
Outdated
Show resolved
Hide resolved
runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/DistributionData.java
Outdated
Show resolved
Hide resolved
runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/DistributionCell.java
Outdated
Show resolved
Hide resolved
runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/DistributionData.java
Outdated
Show resolved
Hide resolved
runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/DistributionData.java
Outdated
Show resolved
Hide resolved
runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/DistributionData.java
Outdated
Show resolved
Hide resolved
028c162 to
21a34e6
Compare
|
LGTM! Thanks for the update. |
21a34e6 to
0399a32
Compare
…ypescript and add GitHub Action Rename node-ts to typescript and add GitHub Action
Problem
This PR intends to add percentiles to beam distribution metric
Motivation
Distributionmetrics in Samza runner are currently not wired to Samza'sMetricsRegistryProposed Changes
DistributionDatacurrently contains the logic for computing distributions. Datasketches library has been used for percentile computation which uses a sketch based approach under the hood. https://datasketches.apache.org/DistributionResultwill report a map of percentiles and percentile values.MetricsRegistryin Samza runnerAPI Changes
public void update(long sum, long count, long min, long max)from Distribution Metric code as it seems to be dead code.Testing
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username).[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
ValidatesRunnercompliance status (on master branch)Examples testing status on various runners
Post-Commit SDK/Transform Integration Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.