NickCraver / StackExchange.Exceptional

Error handler used for the Stack Exchange network
https://nickcraver.com/StackExchange.Exceptional/
Apache License 2.0
863 stars 171 forks source link

Design of Setup method: setting the application name #25

Closed stijnherreman closed 11 years ago

stijnherreman commented 11 years ago

I'm working on some changes to allow SQLErrorStore to display errors from multiple applications.

One issue I'm running into is that the application name is not available during construction of the ErrorStore. Would it make sense to make applicationName part of the constructor? The only side-effect would be that it breaks backwards compatibility.

NickCraver commented 11 years ago

There is a method to do what you're after already, .Setup() - since ApplicationName is global it's not on the store, but set once either from the config or on startup. Here's an example of the code setup version:

ErrorStore.Setup("My App Name", new SQLErrorStore(myConnectionString));

This is so you can setup Exceptional with 0 config lines, all via code.

stijnherreman commented 11 years ago

That's the method I'm using; "My App Name" is not available in the constructor of SqlErrorStore. I've done a quick and dirty implementation already, made the application name part of the constructor and then I do this in the constructor:

public SQLErrorStore(string applicationName, string connectionString, int displayCount = DefaultDisplayCount, int rollupSeconds = DefaultRollupSeconds, string[] visibleApplications = null) : base(rollupSeconds)
{
    _applicationName = applicationName;

    _displayCount = Math.Min(displayCount, MaximumDisplayCount);

    if (connectionString.IsNullOrEmpty()) throw new ArgumentOutOfRangeException("connectionString", "Connection string must be specified when using a SQL error store");
    _connectionString = connectionString;

    _visibleApplications = visibleApplications ?? new[] { ApplicationName };
}

Perhaps there's a better way than my solution to handle this.

Also, unless I missed something, the store can only be changes with Setup() since the Default property is get only. So when you want to change the store, you end up passing the application name again anyway.

NickCraver commented 11 years ago

I don't follow you here... ApplicationName is a static property for everything (very intentional, since it serves as a default which can be overridden - and it may be logged to multiple stores). Being on the constructor of a store doesn't make any sense. If it's from multiple applications then they would all be running their own copy of Exceptional in their app domains, and a static works via .Setup() or config.

If you want to log errors from multiple applications from the same AppDomain, the you can set the applicationName on the error itself. Take a look at the Error() constructor:

public Error(Exception e, HttpContext context, string applicationName = null)
stijnherreman commented 11 years ago

I had forgotten about non-web applications, where you can indeed have multiple stores since you need to manage them yourself (the HTTP module always uses the Default store). I only switched from ELMAH to this yesterday, still learning the ins and outs :)

I suppose what I need instead is the ability to set the Default store after setting the application name. Before I continue, would a pull request (allowing SQLErrorStore to display errors from multiple applications) be considered? If yes, I think I can continue to work on this and any further discussion could be done in the pull request.

NickCraver commented 11 years ago

@stijnherreman displaying from multiple applications (which changes all the queries, and indexing) isn't what Exceptional is designed for...that being said, I have another application in the works that will be open sourced soon as I have time to finish it. This dashboard has many things, one of them being exceptions from multiple applications, (including multiple databases with multiple applications each). You could use only the exceptions portion of this dashboard, and setup is incredibly easy: create an IIS website, copy the files, enter the error store connection strings in the config, done.

Here's a preview Exceptions Dashboard

On the consideration front: I can't see how I'd accept such a pull request, changing a global application name for all stores from the constructor of another store is very weird and unexpected behavior. If setting the application name when adding a second store changed the application name from the first...yeah that'd be very bad, unwanted behavior.

Keep in mind you can call ErrorStore.Setup() (static) at any time to change the default store, and optionally change the application name there as well, in a very intentional/static way. If you don't want to change the application name for everything and just the default store...just keep passing the same application name in.

I think .Setup() already accomplishes what you want, perhaps it's just not immediately clear from the name that it can be called at any time, and multiple times to change things. If that's the case I will certainly update the documentation on it to make this clear.

Please don't take the above as dismissive...if you still have a use case still not covered then let's hop on a google hangout (I can find time next week) and see what we can do to make it work - while helping others in the same boat as well.

stijnherreman commented 11 years ago

The dashboard sure looks interesting. I think I'm not able to clearly explain my goal, so a chat would be useful. Just let me know when you have some time :)

stijnherreman commented 11 years ago

I'll close this, it looks like the dashboard is what we need. I just noticed the Elastic tab, we also happen to use ES. I'm looking forward to an eventual release :)