Skip to content
This repository was archived by the owner on Nov 27, 2018. It is now read-only.

- Removed existing logger scopes as we want to minimize the number of sc... #172

Merged
merged 1 commit into from
Apr 8, 2015
Merged

- Removed existing logger scopes as we want to minimize the number of sc... #172

merged 1 commit into from
Apr 8, 2015

Conversation

kichalla
Copy link
Member

...opes being created.

  • Cleaned up tests related to removal of scopes.

@rynowak

@ghost ghost added the cla-not-required label Mar 24, 2015
@rynowak
Copy link
Member

rynowak commented Mar 24, 2015

/cc @loudej

@rynowak
Copy link
Member

rynowak commented Mar 24, 2015

Removing the scopes is a good start. I think we could even live without the logging in router-middleware and route-collection as well -- @loudej thoughts?

We want logging when there's an important event that happens - in our code that happens inside TemplateRoute pretty much exclusively. The middleware and the collection really just provide aggregation, and that's the kind of thing we want to cut. It starts to feel like tracing -- let's talk if the boundary line here isn't clear.

@rynowak
Copy link
Member

rynowak commented Mar 24, 2015

I'd also really like to see diversification in the kinds of messages logged by TemplateRoute CreateRouteAsyncValues right now produces a one-size-fits-all message. Can you come up with a list of possible scenarios for TemplateRoute and we can think about which data goes with each one?

@kichalla
Copy link
Member Author

Some examples:

  1. A request resulted in desired response
  2. A request resulted in 404 response
    • Possibilities:
      - A request might not have reached the routing middleware itself.
      • Routes did not match due to template mismatch
      • Routes did not match due to constraints failure.
      • Route matched but application layer explicitly returned a 404 (ex: product not found)
      • Route matched but no controller/action present
  3. A request resulted in undesired response.
    • Possiblities:
      • Route matched but resulted in an unexpected controller/action getting executed.

Questions that might come up in general

  • Which route did the request match? or why a particular expected route did not match.
  • if there was a constraint, which one failed and why.

@kichalla kichalla force-pushed the cleanup-logging branch 2 times, most recently from d78d889 to d6cdf6b Compare April 6, 2015 21:29
@kichalla kichalla mentioned this pull request Apr 6, 2015
Merged
rynowak
rynowak reviewed Apr 7, 2015
View reviewed changes
src/Microsoft.AspNet.Routing/RouteConstraintMatcher.cs
Matched = false
});
currentRouteValues = string.Join(", ",
routeValues.Select(rv => $"Key={rv.Key};Value={rv.Value}"));
Copy link
Member

Choose a reason for hiding this comment

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

We don't want to 'encode' values for logging. Just log the value that's relevant to the constraint.

@rynowak
Copy link
Member

rynowak commented Apr 7, 2015

Overall seems on the right track ⌚

@kichalla kichalla force-pushed the cleanup-logging branch 5 times, most recently from b2ef7ad to b034638 Compare April 8, 2015 17:30
rynowak
rynowak reviewed Apr 8, 2015
View reviewed changes
src/Microsoft.AspNet.Routing/RouteConstraintMatcher.cs
if(constraints != null && constraints.Count == 0)
{
logger.LogVerbose("No route constraints found.");
}
Copy link
Member

Choose a reason for hiding this comment

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

None of this should be here

@rynowak
Copy link
Member

rynowak commented Apr 8, 2015

⌚

@davidfowl
Copy link
Member

Why are builds are failing?

@rynowak
Copy link
Member

rynowak commented Apr 8, 2015

@davidfowl stop commenting on PRs and come to the meeting you set up 😆

rynowak
rynowak reviewed Apr 8, 2015
View reviewed changes
src/Microsoft.AspNet.Routing/RouteConstraintMatcher.cs
routeValues.TryGetValue(kvp.Key, out routeValue);

logger.LogVerbose(
"Route value '{RouteValue}' of key '{RouteKey}' did not match " +
Copy link
Member

Choose a reason for hiding this comment

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

of key -> with key

@rynowak
Copy link
Member

rynowak commented Apr 8, 2015

⌚ for test improvements

@rynowak
Copy link
Member

rynowak commented Apr 8, 2015

:shipit:

Think about commonizing the code that you're using to compare dictionaries, this is something that's not trivial in xunit, and you'll be doing a fair bit of it.

@kichalla
Copy link
Member Author

kichalla commented Apr 8, 2015

Thanks...sure will work on creating an assert for dictionaries...

@kichalla
Cleanup logging
1c66e0a 
- Removed existing logger scopes as we want to minimize the number of scopes being created.
- Cleaned up tests related to removal of scopes.
- Added new log statements.
- Removed old logger structure base implementation and related tests. Added new tests also.
@kichalla kichalla merged commit 1c66e0a into dev Apr 8, 2015
@kichalla kichalla deleted the cleanup-logging branch May 1, 2015 16:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kichalla @rynowak @davidfowl