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

Add destructurer for SocketException #249

Closed
TysonMN opened this issue Oct 9, 2020 · 4 comments · Fixed by #253
Closed

Add destructurer for SocketException #249

TysonMN opened this issue Oct 9, 2020 · 4 comments · Fixed by #253
Labels
enhancement Issues describing an enhancement or pull requests adding an enhancement.

Comments

@TysonMN
Copy link
Contributor

TysonMN commented Oct 9, 2020

I suggest adding a destructor for SocketException. I volunteer to do this work, but I also want to do a bit more than the naive implementation.

SocketException has a few properties related to the error (namely ErrorCode, NativeErrorCode, and SocketErrorCode). However, they are all a bit cryptic. The first two are of type int and the last one is of type SocketError, which is an enum. Of course it also has the property HelpLink inherited from Exception. If there was ever a time to set that value, it would be now, but I have only seen it set to null. I think a great help page is the documentation for SocketError, which has a short phrase explaining each case of the enum.

Alongside the other properties of SocketException, I would prefer to also add that URL for the key SocketErrorCodeHelpLink.

How does this sound?

@TysonMN TysonMN added the enhancement Issues describing an enhancement or pull requests adding an enhancement. label Oct 9, 2020
@RehanSaeed
Copy link
Owner

Enriching the exceptions is an interesting idea. I can see only two issues:

  1. Increasing log size is something that worries people. It costs money to store more JSON in ElasticSearch for example.
  2. Microsoft always seems to break their links after a few years.

If you only want to add SocketErrorCodeHelpLink, that doesn't sound too bad but wouldn't it just always have the same link?

@TysonMN
Copy link
Contributor Author

TysonMN commented Oct 15, 2020

Increasing log size is something that worries people. It costs money to store more JSON in ElasticSearch for example.

If an application is throwing (and logging) SocketExceptions so frequently that an extra property and URL is using too much space (whatever the sink), then the application has more important problems to worry about.

Application-level enrichers are common place (like the machine name enricher) and will consume much more space than what I am proposing. Each sink already has to confront this duplication and decide what to do about it. To save on space, a table with the unique values could be created. Then each log event wild contain references to entries in this table. It is a classic time-space tradeoff. I don't think Serilog.Exceptions should limit itself because some sinks optimize for time over space.

However, for those overly concerned about storage space, they can filter out all Serilog.Exceptions with selective enrichment or (I think) just this addition property we are discussing via the property filtering feature of Serilog.Exceptions.

When there is an exception, the default behavior

  1. could be primarily provide lots of useful information and secondarily consider space usage or
  2. could be primarily optimize for little space usage and secondarily squeeze in some useful information.

Like Nicholas says in his selective enrichment post:

Enrichers need to be selected carefully, but more often than not, ease of diagnostics wins over collection costs, and events still end up with a lot of additional properties.

@TysonMN
Copy link
Contributor Author

TysonMN commented Oct 15, 2020

Microsoft always seems to break their links after a few years.

I agree about the fragility of the URL.

In my local implement of this, I didn't log the URL. Instead, I copied the sentence of documentation for each enum case into a dictionary, used the enum value to obtain the sentence of documentation, and enriched that information instead of a URL. This is more useful because it doesn't require an internet connection and doesn't require the loading and searching of a webpage.

How about we do the same here?

@RehanSaeed
Copy link
Owner

That sounds good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issues describing an enhancement or pull requests adding an enhancement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants