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

Send 'error' severity by default & document how to set it #55

Merged
merged 2 commits into from
May 20, 2017
Merged

Conversation

kyrylo
Copy link
Contributor

@kyrylo kyrylo commented May 16, 2017

This is an upcoming feature.

@kyrylo kyrylo force-pushed the severity branch 2 times, most recently from 515d881 to 56c5939 Compare May 16, 2017 12:42
/// Error severity.
/// </summary>
[JsonProperty("severity")]
string Severity { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Plz add public modifier 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.

Fixed.

@kyrylo kyrylo force-pushed the severity branch 8 times, most recently from 5dc7a3a to bc33fa5 Compare May 16, 2017 13:43
@kyrylo
Copy link
Contributor Author

kyrylo commented May 16, 2017

Fixed build, added description to README.

PTAL

@aholovko
Copy link
Contributor

Fixed build, added description to README.

👍

As stated in https://airbrake.io/docs/airbrake-faq/what-is-severity/ Airbrake supports well-defined list of severities (i.e. debug, info, notice, etc.). It makes sense to implement it as an enum that defines all supported values - it should help our customers to choose a proper value and avoid mistakes (that will be for sure in the case when arbitrary string values are allowed).

You can define it in the next way:

public enum Severity
{
    Debug,
    Info,
    // etc...
}

I am not sure also whether it should be a part of IHttpContext. What if we have a console app that doesn't want to deal with HTTP. Shouldn't it have ability to set the severity of error as well?

@kyrylo
Copy link
Contributor Author

kyrylo commented May 16, 2017

👍 to enum

I am not sure also whether it should be a part of IHttpContext. What if we have a console app that doesn't want to deal with HTTP. Shouldn't it have ability to set the severity of error as well?

Anything should be able to set severity. It's a basic feature and every app has its own understand of what's severe and what is not.

@aholovko
Copy link
Contributor

Anything should be able to set severity.

Yep, that's why I propose to set it not via HttpContext but using some other mechanism. One solution is to pass it via parameter to NotifyAsync method with default value set to error, e.g.

NotifyAsync(Exception exception, IHttpContext context = null, Severity = Severity.Error)

If you don't mind, tomorrow we can work together on that part.

@kyrylo
Copy link
Contributor Author

kyrylo commented May 16, 2017

Why would we special case it? Currently, we pass params via HttpContext, but these params are not related to HTTP. They can be, however it's usually a place to store arbitrary info.

I don't see why Severity deserves special treatment. It's just another parameter in the context payload.

@aholovko
Copy link
Contributor

Currently, we pass params via HttpContext, but these params are not related to HTTP.

From what I can see these parameters are HTTP related:

  • Params property from Notice
  • params is An object containing the request parameters. Where the key is the parameter name, e.g. { "page": "3", "sort": "desc" }. from spec
  • implementation in Middleware
  • implementation in Module

@kyrylo
Copy link
Contributor Author

kyrylo commented May 16, 2017

I agree with this rationale. I didn't phrase it correctly. It's a place for HTTP params, yes. However, sometimes we need to store extra information and Params is the default place for that. Anything extra should go there.

NotifyAsync(Exception exception, IHttpContext context = null, Severity = Severity.Error)

^^ this code looks good, as long as we can support other fields (if we need them in the future). For example:

NotifyAsync(Exception exception, IHttpContext context = null, {
    Severity = Severity.Error,
    OtherThing = Val
})

This API looks acceptable. Would you be able to replace this PR with a different one? It'll take me quite some time to implement it.

@aholovko
Copy link
Contributor

However, sometimes we need to store extra information and Params is the default place for that. Anything extra should go there.

If this is a common place where severity is put (or will be put if this is a new feature) for other notifiers, then we should follow that convention as well. And I don't see any problems here. At the same time, we are not forced to use IHttpContext interface for that. We can use some other, more appropriate and less "semantic breaking" mechanism for passing the value of severity to Param collection. Using IHttpContext here also has another drawback - in console application you will be forced to have the full implementation of IHttpContext just to be able to set the value of severity. It seems to be quite inconvenient approach.

this code looks good, as long as we can support other fields (if we need them in the future). For example...

Not sure that we can guarantee "as long as we can support other fields" part. I think that we should consider each case individually and make a decision based on the purpose of each particular field. If there will be a lot of such "custom" fields, perhaps we should come with some solution, but it's quite hard to talk about such solution in advance. Let's come back to error severity. As for me, this is a quite "important" field that deserves its own parameter in the NotifyAsync method. There are a lot of APIs where that kind of information has earned its own method and not just a parameter name (e.g. logger.Error(), logger.Warn(), etc.). I have updated your code to reflect that point. Please check these changes and let me know if you have any notes here.

@kyrylo
Copy link
Contributor Author

kyrylo commented May 17, 2017

Severity should be put in context/severity.

Arbitrary params are useful for user filters. Let's say for some reason you want to send a custom environment variable. You define a new filter, read the value, now you need to attach it to the notice object. The params dict is the correct place for that. It's basically a dumpster for everything.

Not sure that we can guarantee "as long as we can support other fields" part. I think that we should consider each case individually and make a decision based on the purpose of each particular field.

Sounds good.

Please check these changes and let me know if you have any notes here.

Thanks! I have no objections. Hit merge as long as you are happy with the code. After that, we will want to make a new minor release.

@aholovko aholovko merged commit a09c121 into master May 20, 2017
@aholovko aholovko deleted the severity branch May 20, 2017 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants