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 structured logging to FailureItem #1651

Merged
merged 8 commits into from Feb 2, 2018

Conversation

Projects
None yet
3 participants
@andreiruse
Member

andreiruse commented Feb 1, 2018

Making changes to FailureItem in order to use structured logging, provided by Messages.formatWithAttributes().

Added a unit test for this use case.

Making changes to FailureItem in order to use structured logging, pro…
…vided by Messages.formatWithAttributes().

Added a unit test for this use case.

@andreiruse andreiruse requested a review from cjkent Feb 1, 2018

andreiruse added some commits Feb 1, 2018

* The message is produced using a template that contains zero to many "{}" placeholders.
* Each placeholder is replaced by the next available argument.
* The message is produced using a template that contains zero to many "{}" or "{abc}" placeholders.
* Each placeholder is replaced by the next available argument. If the placeholder is name, it will be added to the attributes map.

This comment has been minimized.

@cjkent

cjkent Feb 1, 2018

Member

Typo "is name" should be "is named" or "has a name". Also the rest of the sentence could be clearer. Maybe something like "it's value is added to the attribute map with the name as the key".

@@ -61,6 +63,11 @@
*/
@PropertyDefinition(validate = "notEmpty")
private final String message;
/**
* The attributes associated with this failure.

This comment has been minimized.

@cjkent

cjkent Feb 1, 2018

Member

A bit more detail would be nice here. Something like "Attributes can contain additional information about the failure. For example, a line number in a file or the ID of a trade".

*
* @param reason the reason
* @param message the failure message, not empty
* @param attributes the attributes associated to this failure

This comment has been minimized.

@cjkent

cjkent Feb 1, 2018

Member

"associated with"

* @param skipFrames the number of caller frames to skip, not including this one
* @return the failure
*/
static FailureItem of(FailureReason reason, String message, Map<String, String> attributes, int skipFrames) {

This comment has been minimized.

@cjkent

cjkent Feb 1, 2018

Member

This is package-private. I'm assuming you want it to be public. If so you shouldn't have the skipFrames parameter.

assertEquals(test.getCauseType().isPresent(), true);
assertEquals(test.getCauseType().get(), IllegalArgumentException.class);
assertEquals(test.getStackTrace().contains(".test_of_reasonMessageExceptionNestedExceptionWithAttributes("), true);
assertEquals(test.toString(), "INVALID: my big bad failure: java.lang.IllegalArgumentException: message : {bar=bad, foo=big}");

This comment has been minimized.

@cjkent

cjkent Feb 1, 2018

Member

It would be good to test the attributes map directly rather than doing it via the toString() method.

andreiruse added some commits Feb 1, 2018

* @param skipFrames the number of caller frames to skip, not including this one
* @return the failure
*/
static FailureItem of(FailureReason reason, String message, Map<String, String> attributes, int skipFrames) {
public static FailureItem of(FailureReason reason, String message, Map<String, String> attributes, int skipFrames) {

This comment has been minimized.

@cjkent

cjkent Feb 1, 2018

Member

Get rid of skipFrames, it's in internal implementation detail. Have a look at the other public of methods to see what they do.

* @return the failure
*/
public static FailureItem of(FailureReason reason, String message, Map<String, String> attributes, int skipFrames) {
public static FailureItem of(FailureReason reason, String message, Map<String, String> attributes) {

This comment has been minimized.

@cjkent

cjkent Feb 1, 2018

Member

I've had second thoughts about including this method. I think it might be confusing. What if the user includes named placeholders in the message? Will they expect the attribute values to be substituted? Also, for symmetry you might expect the attribute map to be added to other factory methods. Better to keep things simple and get rid of it seeing as we said we didn't have a use case for it now. We can always add it later if we need to.

P.S. Sorry for changing my mind.

This comment has been minimized.

@andreiruse

andreiruse added some commits Feb 2, 2018

Added the previously removed class back - as its removal changed the …
…overriding dynamic and resulted in failing tests
@cjkent

cjkent approved these changes Feb 2, 2018

@cjkent cjkent merged commit 5fee2a2 into master Feb 2, 2018

15 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
security/snyk No dependency changes
Details
security/snyk - modules/basics/pom.xml No dependency changes
Details
security/snyk - modules/calc/pom.xml No dependency changes
Details
security/snyk - modules/collect/pom.xml No dependency changes
Details
security/snyk - modules/data/pom.xml No dependency changes
Details
security/snyk - modules/loader/pom.xml No dependency changes
Details
security/snyk - modules/market/pom.xml No dependency changes
Details
security/snyk - modules/math/pom.xml No dependency changes
Details
security/snyk - modules/measure/pom.xml No dependency changes
Details
security/snyk - modules/pom.xml No dependency changes
Details
security/snyk - modules/pricer/pom.xml No dependency changes
Details
security/snyk - modules/product/pom.xml No dependency changes
Details
security/snyk - modules/report/pom.xml No dependency changes
Details

@cjkent cjkent deleted the topic/make-failureitem-use-messages-with-attributes branch Feb 2, 2018

@jodastephen jodastephen added this to the v1.7 milestone Feb 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment