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

LogEventInfo.StackTrace moved into CallSiteInformation #2386

Merged
merged 1 commit into from
Nov 18, 2017

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Nov 13, 2017

Attempt to resolve #2382

  • Introduced internal CallsiteInformation-class for handling Callsite-state. Also when using Caller Information Attributes.
  • Deprecated the old calculation of UserStackFrameNumber, but still returns the legacy value for external partners. Instead the method and filenumber-detail is extracted from the MoveNext-MethodBase.
  • Changed Log4JXmlEventLayoutRenderer to handle async-MoveNext out of the box.
  • Added lots of TODOs about removing the use of LogEventInfo.Properties for storing Caller Information Attributes (NLog-fluent)

@snakefoot snakefoot force-pushed the LogEventCallSite branch 3 times, most recently from 6046bd0 to 0852ecc Compare November 13, 2017 22:32
@codecov
Copy link

codecov bot commented Nov 13, 2017

Codecov Report

Merging #2386 into master will decrease coverage by <1%.
The diff coverage is 69%.

@@           Coverage Diff           @@
##           master   #2386    +/-   ##
=======================================
- Coverage      82%     82%   -<1%     
=======================================
  Files         318     319     +1     
  Lines       22796   22898   +102     
  Branches     2789    2824    +35     
=======================================
+ Hits        18671   18705    +34     
- Misses       3419    3465    +46     
- Partials      706     728    +22

@304NotModified 304NotModified added this to the 4.5 beta 8 milestone Nov 17, 2017
/// </summary>
private static HashSet<string> CallerInformationAttributeNames = new HashSet<string>
private static List<string> CallerInformationAttributeNames = new List<string>
Copy link
Member

Choose a reason for hiding this comment

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

why no hashset? Afaik it's more efficient with 3+ elements, and it's static

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well I don't need the HashSet-lookup, and the struct-enumerator on List is allocation-free.

Copy link
Member

@304NotModified 304NotModified left a comment

Choose a reason for hiding this comment

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

looks great!

Two small comments.

return frame?.GetMethod();
}

public string GetCallerClassName(MethodBase method, bool includeNameSpace, bool cleanAsyncMoveNext, bool cleanAnonymousDelegates)
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about a struct/class for the last 3 parameters?

(the same object for GetCallerMemberName

Only for readability of the calling site.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have no opinion. But not that big fan of struct-parameters.

Copy link
Member

Choose a reason for hiding this comment

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

OK,it's internal.so could be done in the future

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

Successfully merging this pull request may close these issues.

2 participants