Skip to content
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

JsonLayout: add includeAllProperties & excludeProperties #1754

Merged

Conversation

ericanastas
Copy link
Contributor

@ericanastas ericanastas commented Nov 10, 2016

Here's an initial attempt at adding "AllEventProperties" functionality to JsonLayout as mentioned in issue #1686. This is the first time I've made a pull request on GitHub. So if I'm not following the proper procedure or conventions let me know.

I have few questions:

  1. I'm using a for each loop to iterate over the the log event properties, but everywhere else it looks like there's been an effort to use a for loop to avoid allocating an Enumerator. There doesn't look like a way to access the items or keys in a dictionary by index though. Is there some other way I could optimize this?

  2. LogEventInfo.Properties is an IDictionary<object,object>. How should the keys be converted into a string property names? Should I just use ToString() as I have now? When would the key of a property not be a string?

  3. Any ideas on how to handle property values that are collections or objects? I'm guessing we wouldn't want to use reflection to read properties from object values due to the performance hit. However, for my use case I'd like it to be able to identify collection properties and have them returned as an array of values in the JSON rather then "System.Collections.Generic.List`1[System.String]".


This change is Reviewable

@codecov-io
Copy link

Current coverage is 80% (diff: 100%)

Merging #1754 into master will increase coverage by <1%

@@             master      #1754   diff @@
==========================================
  Files           274        274          
  Lines         17422      17472    +50   
  Methods        2741       2743     +2   
  Messages          0          0          
  Branches       1966       1981    +15   
==========================================
+ Hits          13974      14024    +50   
  Misses         3005       3005          
  Partials        443        443          

Sunburst

Powered by Codecov. Last update 71c8b60...90757ff

/// <summary>
/// List of property names to exclude when IncludeAllProperties is true
/// </summary>
public IList<string> ExcludedProperties { get; set; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't this will work from the XML, yet!

See #1439 (but I won't support IList).

Is it possible to write an unit test for this, so we won't forget it? (and it will succeed if we rebase after #1439 has been finished)

});
}
else if (
prop.Value is bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should integrate this with #1740 (injecting Json serializer) . What do you think?

@304NotModified
Copy link
Member

304NotModified commented Nov 13, 2016

I'm using a for each loop to iterate over the the log event properties, but everywhere else it looks like there's been an effort to use a for loop to avoid allocating an Enumerator. There doesn't look like a way to access the items or keys in a dictionary by index though. Is there some other way I could optimize this?

We try to use for loops if we found in tests that would benefit. I think we could loop over the keys in this case, but I doubt if that will be an improvement. TL;DR, it's fine with a foreach

LogEventInfo.Properties is an IDictionary<object,object>. How should the keys be converted into a string property names? Should I just use ToString() as I have now? When would the key of a property not be a string?

preferred to use the JsonSerializer of #1740?

update: we have to be careful to be backwardscompatibe.

Any ideas on how to handle property values that are collections or objects?

Also #1740?

@304NotModified 304NotModified added this to the 4.4 milestone Nov 13, 2016
@304NotModified
Copy link
Member

Intent to release this nice feature into 4.4 :)

@304NotModified
Copy link
Member

We need to do the docs in wiki right?

@304NotModified
Copy link
Member

@304NotModified 304NotModified changed the title JsonLayout.AllEventProperties JsonLayout: add includeAllProperties & excludeProperties Nov 24, 2016
@304NotModified 304NotModified added the documentation done all docs done (wiki, api docs, lists on nlog-project.org, xmldocs) label Nov 24, 2016
@AuthorProxy
Copy link
Contributor

needs to update xsd schema now, cause The attribute is not declared

@304NotModified
Copy link
Member

thanks @AuthorProxy , created a new issue for it: #2037

@304NotModified
Copy link
Member

@AuthorProxy It seems that nlog.schema was up-to-date, but not the one on nlog-project.org. This has been fixed.

Have to find out how to do this automatically. (powershell script or something like that)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation done all docs done (wiki, api docs, lists on nlog-project.org, xmldocs) feature json / json-layout needs documentation on wiki
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants