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.

Sign up for GitHub

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

Jump to bottom

NDLC - Perform low resolution scope timing #2377

Merged
merged 1 commit into from
Nov 17, 2017
Merged

NDLC - Perform low resolution scope timing #2377

merged 1 commit into from
Nov 17, 2017

Conversation

snakefoot
Copy link
Contributor

Implemented async scope timing

@snakefoot snakefoot force-pushed the ndlctimingrenderer branch 2 times, most recently from 3754573 to 2f76e0b Compare October 30, 2017 20:24
@304NotModified 304NotModified self-assigned this Oct 30, 2017
@snakefoot snakefoot force-pushed the ndlctimingrenderer branch 2 times, most recently from 0220942 to f8b0146 Compare October 30, 2017 20:41
@codecov
Copy link

codecov bot commented Oct 30, 2017 •
edited
Loading

Codecov Report

Merging #2377 into master will decrease coverage by <1%.
The diff coverage is 89%.

@@           Coverage Diff           @@
##           master   #2377    +/-   ##
=======================================
- Coverage      82%     82%   -<1%     
=======================================
  Files         318     319     +1     
  Lines       22796   22842    +46     
  Branches     2789    2802    +13     
=======================================
+ Hits        18671   18692    +21     
- Misses       3419    3426     +7     
- Partials      706     724    +18

304NotModified
304NotModified reviewed Nov 2, 2017
View reviewed changes
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.

could you please explain me in short why we need this? thx!

src/NLog/LayoutRenderers/NdlcTimingLayoutRenderer.cs Outdated
#if !NET3_5
builder.Append(duration.ToString(Format, formatProvider));
#else
builder.Append(duration.ToString());
Copy link
Member

Choose a reason for hiding this comment

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

formatProvider not needed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no TimeSpan.ToString with Format or formatProvider in NET3_5

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, but I think the call var formatProvider = GetFormatProvider(logEvent, null); is not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

src/NLog/NestedDiagnosticsLogicalContext.cs Outdated
{
var currentContext = GetThreadLocal();
if (currentContext == null)
return DateTime.MinValue;
Copy link
Member

Choose a reason for hiding this comment

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

what about nullable datetimes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer DateTime.MinValue as magic value, don't need another one.

src/NLog/NestedDiagnosticsLogicalContext.cs Outdated
/// Peeks the current scope, and returns its start time
/// </summary>
/// <returns>Scope Creation Time</returns>
public static DateTime PeekCurrentScopeBeginTime()
Copy link
Member

Choose a reason for hiding this comment

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

could this be internal? (2x)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@304NotModified 304NotModified added this to the 4.5 beta 8 milestone Nov 2, 2017
@snakefoot
Copy link
Contributor Author

snakefoot commented Nov 2, 2017 •
edited
Loading

could you please explain me in short why we need this? thx!

If you create a scope to mark that you have started a web-request, then you can use this to extract the timings for this scoped web-request (when logging messages that belongs to the async web-request). Maybe also use this information to perform filtering of web-requests, so logging those starting to take longer than 1 sec.

I guess one could also implement this by calling NDLC.Push with a complex object, but I guess we then need to provide a PeekTop and PeekBottom to retrieve this object in a custom layout-renderer.

@snakefoot snakefoot force-pushed the ndlctimingrenderer branch 9 times, most recently from a3ea8fb to 6c81ea4 Compare November 6, 2017 21:36
304NotModified
304NotModified approved these changes Nov 6, 2017
View reviewed changes
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.

thx!

@304NotModified 304NotModified merged commit 0d842b6 into NLog:master Nov 17, 2017
@snakefoot snakefoot mentioned this pull request Nov 23, 2017
Closed
@snakefoot snakefoot modified the milestones: 4.5 beta 8, 4.5 Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@304NotModified 304NotModified 304NotModified approved these changes

Assignees

@304NotModified 304NotModified

Labels
feature
Projects
None yet
Milestone
4.5
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@snakefoot @304NotModified