You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@samza.apache.org by GitBox <gi...@apache.org> on 2020/08/14 00:06:00 UTC
[GitHub] [samza] vishwajith-s opened a new pull request #1416: SAMZA-2581: Fix Samza Histogram update to update percentile Gauge values
vishwajith-s opened a new pull request #1416:
URL: https://github.com/apache/samza/pull/1416
Symptom: Missing metrics. EventSystemConsumer consumer lag latency
metrics are missing.
Cause: Histogram update and percentile gauges are updated in two
different functions. Percentiles are internal to Samza histogram and
hence should be updated internally as part of update.
Changes: Histogram update also updates internal gauges.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] prateekm commented on pull request #1416: SAMZA-2581: Fix Samza Histogram update to update percentile Gauge values
Posted by GitBox <gi...@apache.org>.
prateekm commented on pull request #1416:
URL: https://github.com/apache/samza/pull/1416#issuecomment-675025769
@vishwajith-s Can you add an inline comment or update the description of the PR explaining why we chose this impl (i.e. document what we discussed)? Otherwise LGTM, thanks for fixing this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] vishwajith-s commented on pull request #1416: SAMZA-2581: Fix Samza Histogram update to update percentile Gauge values
Posted by GitBox <gi...@apache.org>.
vishwajith-s commented on pull request #1416:
URL: https://github.com/apache/samza/pull/1416#issuecomment-675040927
@prateekm @xiefan46 can you please merge my PR? I don't have the permissions.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] vishwajith-s commented on pull request #1416: SAMZA-2581: Fix Samza Histogram update to update percentile Gauge values
Posted by GitBox <gi...@apache.org>.
vishwajith-s commented on pull request #1416:
URL: https://github.com/apache/samza/pull/1416#issuecomment-673778150
> Coud you please add some comments to the histogram variable and the gauges variable to briefly explain what are they used for? These two variables looks confusing and may cause bugs in the future.
The class already has explanation for the Gauge. Custom gauge whose value is set based on the underlying Histogram
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] vishwajith-s commented on a change in pull request #1416: SAMZA-2581: Fix Samza Histogram update to update percentile Gauge values
Posted by GitBox <gi...@apache.org>.
vishwajith-s commented on a change in pull request #1416:
URL: https://github.com/apache/samza/pull/1416#discussion_r470349757
##########
File path: samza-api/src/main/java/org/apache/samza/metrics/SamzaHistogram.java
##########
@@ -53,6 +53,7 @@ public SamzaHistogram(MetricsRegistry registry, String group, String name, List<
public void update(long value) {
histogram.update(value);
+ percentiles.forEach(x -> updateGaugeValues(x));
Review comment:
good point. @prateekm is it possible that this is never called? We are seeing the values a 0, so I assumed this could be the issue.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] prateekm commented on a change in pull request #1416: SAMZA-2581: Fix Samza Histogram update to update percentile Gauge values
Posted by GitBox <gi...@apache.org>.
prateekm commented on a change in pull request #1416:
URL: https://github.com/apache/samza/pull/1416#discussion_r471701780
##########
File path: samza-api/src/main/java/org/apache/samza/metrics/SamzaHistogram.java
##########
@@ -75,5 +75,15 @@ public void visit(MetricsVisitor visitor) {
updateGaugeValues(percentile);
Review comment:
Should this be removed from here if this is already being updated in getValue?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] xiefan46 commented on a change in pull request #1416: SAMZA-2581: Fix Samza Histogram update to update percentile Gauge values
Posted by GitBox <gi...@apache.org>.
xiefan46 commented on a change in pull request #1416:
URL: https://github.com/apache/samza/pull/1416#discussion_r470332203
##########
File path: samza-api/src/main/java/org/apache/samza/metrics/SamzaHistogram.java
##########
@@ -53,6 +53,8 @@ public SamzaHistogram(MetricsRegistry registry, String group, String name, List<
public void update(long value) {
histogram.update(value);
+ Snapshot values = histogram.getSnapshot();
Review comment:
Looks like this line is unnecessary? The "values" variable is never being used.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] prateekm merged pull request #1416: SAMZA-2581: Fix Samza Histogram update to update percentile Gauge values
Posted by GitBox <gi...@apache.org>.
prateekm merged pull request #1416:
URL: https://github.com/apache/samza/pull/1416
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] xiefan46 commented on pull request #1416: SAMZA-2581: Fix Samza Histogram update to update percentile Gauge values
Posted by GitBox <gi...@apache.org>.
xiefan46 commented on pull request #1416:
URL: https://github.com/apache/samza/pull/1416#issuecomment-673780355
lgtm
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] vishwajith-s commented on a change in pull request #1416: SAMZA-2581: Fix Samza Histogram update to update percentile Gauge values
Posted by GitBox <gi...@apache.org>.
vishwajith-s commented on a change in pull request #1416:
URL: https://github.com/apache/samza/pull/1416#discussion_r471562099
##########
File path: samza-api/src/main/java/org/apache/samza/metrics/SamzaHistogram.java
##########
@@ -53,6 +53,7 @@ public SamzaHistogram(MetricsRegistry registry, String group, String name, List<
public void update(long value) {
histogram.update(value);
+ percentiles.forEach(x -> updateGaugeValues(x));
Review comment:
@prateekm as discussed doing it in the getValue of the Histogram.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] xiefan46 commented on pull request #1416: SAMZA-2581: Fix Samza Histogram update to update percentile Gauge values
Posted by GitBox <gi...@apache.org>.
xiefan46 commented on pull request #1416:
URL: https://github.com/apache/samza/pull/1416#issuecomment-673776452
Coud you please add some comments to the histogram variable and the gauges variable to briefly explain what are they used for? These two variables looks confusing and may cause bugs in the future.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] vishwajith-s commented on a change in pull request #1416: SAMZA-2581: Fix Samza Histogram update to update percentile Gauge values
Posted by GitBox <gi...@apache.org>.
vishwajith-s commented on a change in pull request #1416:
URL: https://github.com/apache/samza/pull/1416#discussion_r470355265
##########
File path: samza-api/src/main/java/org/apache/samza/metrics/SamzaHistogram.java
##########
@@ -53,6 +53,7 @@ public SamzaHistogram(MetricsRegistry registry, String group, String name, List<
public void update(long value) {
histogram.update(value);
+ percentiles.forEach(x -> updateGaugeValues(x));
Review comment:
Also by the way this code is being called from Brooklin code in Brooklin cluster. Not sure if the visit is called
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] prateekm commented on a change in pull request #1416: SAMZA-2581: Fix Samza Histogram update to update percentile Gauge values
Posted by GitBox <gi...@apache.org>.
prateekm commented on a change in pull request #1416:
URL: https://github.com/apache/samza/pull/1416#discussion_r470346356
##########
File path: samza-api/src/main/java/org/apache/samza/metrics/SamzaHistogram.java
##########
@@ -53,6 +53,7 @@ public SamzaHistogram(MetricsRegistry registry, String group, String name, List<
public void update(long value) {
histogram.update(value);
+ percentiles.forEach(x -> updateGaugeValues(x));
Review comment:
Looks like the gauge value is already updated when it is reported (line 76 Gauge#visit). Why do we need to update them on every new datapoint?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org