Logging field specification and value extraction errors #519

lorenaqendro · 2018-03-06T16:58:28Z

Description

add exception when illegal args are included in the recipe
add exception when the field cannot be computed
change color of error log in the data export runner
renaming in percentile field
update tests

Fixes #269

Checklist

~~Created new test(s) (if applicable)~~
Updated tests
~~Updated the README / documentation (if applicable)~~

- add exception when the field cannot be computed - change color of error log in the data export runner - renaming in percentile field

arya-hemanshu

Looks good in general, please look at the comments.

arya-hemanshu · 2018-03-07T11:58:29Z

src/main/java/uk/org/tombolo/field/aggregation/BackOffField.java

            try {
                value = ((SingleValueField) field).valueForSubject(subject, timeStamp);
            } catch (IncomputableFieldException e){
                // Keep calm and continue processing ... we will back-off
                continue;
+            } catch (ClassCastException cce) {
+                throw new IllegalArgumentException("Parameters for BackOffField must be of type SingleValueField.");


I think exception here is redundant, as we are already catching that in initialize

Catching a different one here.

which one? I think we are looping over the same fields which are added through intialize method

ClassCastException here and ClassNotFoundException in initialize().

my bad, i was always reading class. Need to start reading class names more thoroughly :)

arya-hemanshu · 2018-03-07T12:28:39Z

src/main/java/uk/org/tombolo/field/transformation/DescriptiveStatisticsField.java

@@ -71,11 +71,11 @@ protected void initialize(){
            try {
                Field field = fieldRecipe.toField();
                if (!(field instanceof SingleValueField))
-                    throw new IncomputableFieldException("Parameters for DescriptiveStatisticsField must be of type SingleValueField");
+                    throw new IllegalArgumentException("Parameters for DescriptiveStatisticsField must be of type SingleValueField");


Earlier we were trying to cast to field and then throwing as cast exception, here we are using a different approach for same thing, it feels to me a little inconsistent

I wouldn't say it's inconsistent as it expresses a different way of doing the same thing, which does not manifest inconsistency in the behaviour itself. I would say that both ways are acceptable. :)

It's the consistency of writing code i was talking about not the intended behaviour. It just looks more neat. However it's okay if you would prefer this way

arya-hemanshu · 2018-03-07T12:32:48Z

src/main/java/uk/org/tombolo/field/transformation/LinearCombinationField.java


        try {
            singleValueFields = new ArrayList<>();

            for (FieldRecipe fieldRecipe: fields) {
                Field field = fieldRecipe.toField();
                if (!(field instanceof SingleValueField))
-                    throw new IncomputableFieldException("Parameters for LinearCombinationField must be of type SingleValueField");
+                    throw new IllegalArgumentException("Parameters for LinearCombinationField must be of type SingleValueField");


It would be good if we follow the same approach of cast and catching the exception, just to be consistent

arya-hemanshu · 2018-03-07T12:58:41Z

src/main/java/uk/org/tombolo/field/value/BasicValueField.java

+                throw new IncomputableFieldException(String.format("No attribute found for provider %s and label %s", attribute.provider, attribute.label));
+            } else {
+                attributeObject = attr;
+            }


isn't that the attributeObject will always be null when this field is called as the object is private and is not initialised. Can we write it like

Attribute attr = AttributeUtils.getByProviderAndLabel(attribute.provider, attribute.label); if (null == attr) { throw new IncomputableFieldException(String.format("No attribute found for provider %s and label %s", attribute.provider, attribute.label)); } return attr;

and remove the above attributeObject

arya-hemanshu · 2018-03-07T13:02:02Z

src/main/java/uk/org/tombolo/field/value/TimeseriesField.java

+        if (timedValues.isEmpty()) {
+            throw new IncomputableFieldException(String.format("No TimedValue found for attribute %s", getAttribute().getLabel()));
+        }
+        arr.addAll(timedValues.stream().map(timedValue -> {
            JSONObject pair = new JSONObject();


If it's empty we can just return after throwing the exception

It won't get far after the exception is thrown. What I can do is create the json array object after so we don't waste one.

arya-hemanshu · 2018-03-07T13:03:30Z

src/main/java/uk/org/tombolo/field/value/TimeseriesMeanValueField.java

+        if (timedValue.isEmpty()) {
+            throw new IncomputableFieldException(String.format("No TimedValue found for attribute %s", getAttribute().getLabel()));
+        }
+


we can return from here if it is empty

arya-hemanshu · 2018-03-07T13:07:43Z

src/main/java/uk/org/tombolo/exporter/CSVExporter.java

@@ -69,8 +69,12 @@ public void write(Writer writer, List<Subject> subjects, List<Field> fields, Boo
 			try {
 				property.put(field.getLabel(), ((SingleValueField) field).valueForSubject(subject, timeStamp));
 			} catch (IncomputableFieldException e) {
-				log.warn("Could not compute Field {} for Subject {}, reason: {}", field.getLabel(), subject.getLabel(), e.getMessage());
+				log.warn("\u001b[0;33m" + "Could not compute Field {} for Subject {}, reason: {}" + "\u001b[m",
+						field.getLabel(), subject.getLabel(), e.getMessage());


It would be good if we could have a constant class containing these formats, so that we dont have to write them again and again when required, we could just refer to those constants.

lorenaqendro · 2018-03-12T16:39:12Z

@arya-hemanshu can you have another look?

arya-hemanshu

looks good to merge

lorenaqendro added 2 commits March 6, 2018 15:54

Add exception when illegal args are included in the recipe

49ebf36

- add exception when the field cannot be computed - change color of error log in the data export runner - renaming in percentile field

update tests

e9be2d6

lorenaqendro requested a review from arya-hemanshu March 6, 2018 17:08

arya-hemanshu reviewed Mar 7, 2018

View reviewed changes

lorenaqendro and others added 2 commits March 12, 2018 16:38

address comments

37fcd3d

Merge branch 'master' into lq-269-log_err_math_op_strings

84e8290

Merge branch 'master' into lq-269-log_err_math_op_strings

7e3b860

arya-hemanshu approved these changes Mar 13, 2018

View reviewed changes

lorenaqendro merged commit 3de0887 into master Mar 13, 2018

lorenaqendro deleted the lq-269-log_err_math_op_strings branch March 13, 2018 10:26

lorenaqendro added this to the Release Candidate milestone Mar 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Logging field specification and value extraction errors #519

Logging field specification and value extraction errors #519

lorenaqendro commented Mar 6, 2018

arya-hemanshu left a comment

arya-hemanshu Mar 7, 2018

lorenaqendro Mar 7, 2018

arya-hemanshu Mar 7, 2018

lorenaqendro Mar 7, 2018

arya-hemanshu Mar 12, 2018

arya-hemanshu Mar 7, 2018

lorenaqendro Mar 7, 2018

arya-hemanshu Mar 7, 2018

arya-hemanshu Mar 7, 2018

arya-hemanshu Mar 7, 2018

arya-hemanshu Mar 7, 2018

lorenaqendro Mar 7, 2018

arya-hemanshu Mar 7, 2018

arya-hemanshu Mar 7, 2018

lorenaqendro commented Mar 12, 2018

arya-hemanshu left a comment

Logging field specification and value extraction errors #519

Logging field specification and value extraction errors #519

Conversation

lorenaqendro commented Mar 6, 2018

Description

Checklist

arya-hemanshu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorenaqendro commented Mar 12, 2018

arya-hemanshu left a comment

Choose a reason for hiding this comment