Hadoop Common
  1. Hadoop Common
  2. HADOOP-7627

Improve MetricsAsserts to give more understandable output on failure

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.23.0
    • Fix Version/s: 0.23.0
    • Component/s: metrics, test
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      In developing a test case that uses MetricsAsserts, I had two issues:
      1) the error output in the case that an assertion failed does not currently give any information as to the actual value of the metric
      2) there is no way to retrieve the metric variable (eg to assert that the sum of a metric over all DNs is equal to some value)

      This JIRA is to improve this test class to fix the above issues.

        Activity

        Hide
        Hudson added a comment -

        Integrated in Hadoop-Mapreduce-trunk #854 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/854/)
        HADOOP-7627. Improve MetricsAsserts to give more understandable output on failure. Contributed by Todd Lipcon.

        todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1180259
        Files :

        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/test/MetricsAsserts.java
        Show
        Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #854 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/854/ ) HADOOP-7627 . Improve MetricsAsserts to give more understandable output on failure. Contributed by Todd Lipcon. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1180259 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/test/MetricsAsserts.java
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Mapreduce-0.23-Build #40 (See https://builds.apache.org/job/Hadoop-Mapreduce-0.23-Build/40/)
        HADOOP-7627. Improve MetricsAsserts to give more understandable output on failure. Contributed by Todd Lipcon.

        todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1180258
        Files :

        • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt
        • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/test/MetricsAsserts.java
        Show
        Hudson added a comment - Integrated in Hadoop-Mapreduce-0.23-Build #40 (See https://builds.apache.org/job/Hadoop-Mapreduce-0.23-Build/40/ ) HADOOP-7627 . Improve MetricsAsserts to give more understandable output on failure. Contributed by Todd Lipcon. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1180258 Files : /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/test/MetricsAsserts.java
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-0.23-Build #33 (See https://builds.apache.org/job/Hadoop-Hdfs-0.23-Build/33/)
        HADOOP-7627. Improve MetricsAsserts to give more understandable output on failure. Contributed by Todd Lipcon.

        todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1180258
        Files :

        • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt
        • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/test/MetricsAsserts.java
        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-0.23-Build #33 (See https://builds.apache.org/job/Hadoop-Hdfs-0.23-Build/33/ ) HADOOP-7627 . Improve MetricsAsserts to give more understandable output on failure. Contributed by Todd Lipcon. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1180258 Files : /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/test/MetricsAsserts.java
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk #824 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/824/)
        HADOOP-7627. Improve MetricsAsserts to give more understandable output on failure. Contributed by Todd Lipcon.

        todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1180259
        Files :

        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/test/MetricsAsserts.java
        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #824 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/824/ ) HADOOP-7627 . Improve MetricsAsserts to give more understandable output on failure. Contributed by Todd Lipcon. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1180259 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/test/MetricsAsserts.java
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Mapreduce-trunk-Commit #1061 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/1061/)
        HADOOP-7627. Improve MetricsAsserts to give more understandable output on failure. Contributed by Todd Lipcon.

        todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1180259
        Files :

        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/test/MetricsAsserts.java
        Show
        Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #1061 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/1061/ ) HADOOP-7627 . Improve MetricsAsserts to give more understandable output on failure. Contributed by Todd Lipcon. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1180259 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/test/MetricsAsserts.java
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Common-trunk-Commit #1042 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/1042/)
        HADOOP-7627. Improve MetricsAsserts to give more understandable output on failure. Contributed by Todd Lipcon.

        todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1180259
        Files :

        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/test/MetricsAsserts.java
        Show
        Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #1042 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/1042/ ) HADOOP-7627 . Improve MetricsAsserts to give more understandable output on failure. Contributed by Todd Lipcon. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1180259 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/test/MetricsAsserts.java
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk-Commit #1120 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/1120/)
        HADOOP-7627. Improve MetricsAsserts to give more understandable output on failure. Contributed by Todd Lipcon.

        todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1180259
        Files :

        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/test/MetricsAsserts.java
        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #1120 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/1120/ ) HADOOP-7627 . Improve MetricsAsserts to give more understandable output on failure. Contributed by Todd Lipcon. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1180259 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/test/MetricsAsserts.java
        Hide
        Todd Lipcon added a comment -

        Committed to 23 and trunk, thanks for review, Luke.

        Show
        Todd Lipcon added a comment - Committed to 23 and trunk, thanks for review, Luke.
        Hide
        Luke Lu added a comment -

        +1. We can probably fix this by adding/tweaking the add*(MetricsInfo, Number) APIs. But I let's not go there for now.

        Show
        Luke Lu added a comment - +1. We can probably fix this by adding/tweaking the add*(MetricsInfo, Number) APIs. But I let's not go there for now.
        Hide
        Todd Lipcon added a comment -

        Hi Luke. I tried to make this change, but it doesn't work. The issue I'm getting is this:

        [ERROR] /home/todd/git/hadoop-common/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/test/MetricsAsserts.java:[189,26] cannot find symbol
        [ERROR] symbol : method addGauge(org.apache.hadoop.metrics2.MetricsInfo,T)
        [ERROR] location: class org.apache.hadoop.metrics2.MetricsRecordBuilder

        Upon thinking about it, this makes sense - there's no type qualification on T, so there's no way it can resolve this method call to one of the specific overloads of addGauge in MetricsRecordBuilder. It would work with C++ templates, but not Java generics. So, I think the original approach is the best we can do.

        Show
        Todd Lipcon added a comment - Hi Luke. I tried to make this change, but it doesn't work. The issue I'm getting is this: [ERROR] /home/todd/git/hadoop-common/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/test/MetricsAsserts.java: [189,26] cannot find symbol [ERROR] symbol : method addGauge(org.apache.hadoop.metrics2.MetricsInfo,T) [ERROR] location: class org.apache.hadoop.metrics2.MetricsRecordBuilder Upon thinking about it, this makes sense - there's no type qualification on T, so there's no way it can resolve this method call to one of the specific overloads of addGauge in MetricsRecordBuilder. It would work with C++ templates, but not Java generics. So, I think the original approach is the best we can do.
        Hide
        Luke Lu added a comment -

        Yep. So you don't have to duplicate implementations for all the value types.

        Show
        Luke Lu added a comment - Yep. So you don't have to duplicate implementations for all the value types.
        Hide
        Todd Lipcon added a comment -

        hm, you mean something like changing:

        +  public static double getDoubleGauge(String name, MetricsRecordBuilder rb) {
        +    ArgumentCaptor<Double> captor = ArgumentCaptor.forClass(Double.class);
        +    verify(rb, atLeast(0)).addGauge(eqName(info(name, "")), captor.capture());
        +    checkCaptured(captor, name);
        +    return captor.getValue();
           }
        

        to more like

        public static <T> T getGauge(String name, MetricsRecordBuilder rb, Class<T> clazz) {
          ArgumentCaptor<T> captor = ArgumentCaptor.forClass(clazz);
          verify(rb, atLeast(0)).addGauge(eqName(info(name, "")), captor.capture());
          checkCaptured(captor, name);
          return captor.getValue();
        }
        

        ?

        Show
        Todd Lipcon added a comment - hm, you mean something like changing: + public static double getDoubleGauge( String name, MetricsRecordBuilder rb) { + ArgumentCaptor< Double > captor = ArgumentCaptor.forClass( Double .class); + verify(rb, atLeast(0)).addGauge(eqName(info(name, "")), captor.capture()); + checkCaptured(captor, name); + return captor.getValue(); } to more like public static <T> T getGauge( String name, MetricsRecordBuilder rb, Class <T> clazz) { ArgumentCaptor<T> captor = ArgumentCaptor.forClass(clazz); verify(rb, atLeast(0)).addGauge(eqName(info(name, "")), captor.capture()); checkCaptured(captor, name); return captor.getValue(); } ?
        Hide
        Luke Lu added a comment -

        This is a useful improvement, Thanks Todd. The get*Value implementation can be improved a little to use a generic class though.

        Show
        Luke Lu added a comment - This is a useful improvement, Thanks Todd. The get*Value implementation can be improved a little to use a generic class though.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12494141/hadoop-7627.txt
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 3 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed unit tests in .

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/166//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/166//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12494141/hadoop-7627.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed unit tests in . +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/166//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/166//console This message is automatically generated.
        Hide
        Todd Lipcon added a comment -

        Attached patch fixes the above example to:

        java.lang.AssertionError: Bad value for metric FilesRenamed expected:<2> but was:<1>

        for the incorrect metric case, and:

        java.lang.AssertionError: Expected exactly one metric for name FilesRenamedXXXX expected:<1> but was:<0>

        for the wrong metric name case.

        I tested that TestNameNodeMetrics passes with this change.

        Show
        Todd Lipcon added a comment - Attached patch fixes the above example to: java.lang.AssertionError: Bad value for metric FilesRenamed expected:<2> but was:<1> for the incorrect metric case, and: java.lang.AssertionError: Expected exactly one metric for name FilesRenamedXXXX expected:<1> but was:<0> for the wrong metric name case. I tested that TestNameNodeMetrics passes with this change.
        Hide
        Todd Lipcon added a comment -

        For example, if I break one of the asserts in TestNameNodeMetrics to assert the wrong value, I get the following test failure:

        Argument(s) are different! Wanted:
        metricsRecordBuilder.addCounter(
            Info with name=FilesRenamed,
            2
        );
        -> at org.apache.hadoop.test.MetricsAsserts.assertCounter(MetricsAsserts.java:186)
        Actual invocation has different arguments:
        metricsRecordBuilder.addCounter(
            MetricsInfoImpl{name=CreateFileOps, description=CreateFileOps},
            2
        );
        

        The "actual" result has nothing to do with the metric in question.

        If I break the metric name to be incorrect (eg change FilesRenamed to FilesRenameXXXX) I get the same error trace.

        Show
        Todd Lipcon added a comment - For example, if I break one of the asserts in TestNameNodeMetrics to assert the wrong value, I get the following test failure: Argument(s) are different! Wanted: metricsRecordBuilder.addCounter( Info with name=FilesRenamed, 2 ); -> at org.apache.hadoop.test.MetricsAsserts.assertCounter(MetricsAsserts.java:186) Actual invocation has different arguments: metricsRecordBuilder.addCounter( MetricsInfoImpl{name=CreateFileOps, description=CreateFileOps}, 2 ); The "actual" result has nothing to do with the metric in question. If I break the metric name to be incorrect (eg change FilesRenamed to FilesRenameXXXX) I get the same error trace.

          People

          • Assignee:
            Todd Lipcon
            Reporter:
            Todd Lipcon
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development