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

LogHttpDependency() - allow status codes within the range of 100 to 599 #516

Closed
mic-hnk opened this issue Jan 16, 2023 · 9 comments
Closed
Assignees
Labels
dependencies All issues related to dependencies enhancement New feature or request
Milestone

Comments

@mic-hnk
Copy link
Contributor

mic-hnk commented Jan 16, 2023

Is your feature request related to a problem? Please describe.
One of http dependencies on my application returns status codes outside System.Net.HttpStatusCode enum range for some client errors.
It results in exceptions being thrown by the Guard rule:
Guard.For(() => !Enum.IsDefined(typeof(HttpStatusCode), statusCode), new ArgumentException("Requires a response HTTP status code that's within the bound of the enumeration to track a HTTP dependency"));

Describe the solution you'd like
I would like the extension method to allow status codes according to https://www.rfc-editor.org/rfc/rfc9110#name-status-codes

All valid status codes are within the range of 100 to 599, inclusive.
...
client MUST understand the class of any status code, as indicated by the first digit, and treat an unrecognized status code as being equivalent to the x00 status code of that class.

Describe alternatives you've considered
Alternatively, maybe logging component should not be busy with guarding http status codes. I would expect the http dependency entry to be logged regardless of the status code.
The solution would be to remove status code related guards from the discussed extension methods.

Additional context
I am happy to prepare a PR.
Possible code change: #515

@fgheysels
Copy link
Member

fgheysels commented Jan 16, 2023

I agree that we should not be too restrictive here. Complying with the RFC would indeed be better then relying on the pre-defined status codes in the .NET lib.

But, the LogDependency methods accept a HttpStatusCode enum value, and not an int. Do we need an overload that accepts an int as statuscode ?

@stijnmoreels stijnmoreels added enhancement New feature or request dependencies All issues related to dependencies labels Jan 17, 2023
@stijnmoreels stijnmoreels added this to the v2.8.0 milestone Jan 17, 2023
@stijnmoreels
Copy link
Member

Thank you for your issue, @mic-hnk ! And your effort to fix this yourself.
Want to point out that if you use our new W3C HTTP correlation, that the HTTP dependency is tracked automatically by Microsoft.
To fix this issue, we can indeed provide an overload with int like we do with the LogRequest overloads.
The enum guard here seems like a good thing, as it indeed validates the type.

@fgheysels
Copy link
Member

I agree with @mic-hnk that we should not be too harsh on checking if the http status code is a member of the HttpSTatusCode enum. Especially since an integer for which there's no enum value defined can be casted to a HttpStatusCode member.

Moreover, in my opinion services should not fail or crash because of input-validation on cross cutting concerns like logging.

@mic-hnk
Copy link
Contributor Author

mic-hnk commented Jan 17, 2023

It depends on what you want to achieve.
My short term goal is to have my application fixed and int overload would do just that. I can make necessary code changes.

In broader sense though there can be a server which replies with a valid http code which would break your library. It's not ideal that thing outside my control (server response's status code) results in an exception. - just a personal opinion ;)

Make a final call please and I will move on.

@fgheysels
Copy link
Member

fgheysels commented Jan 17, 2023

I agree with your opinion @mic-hnk . :)
We must absolutely avoid that a program / service / .... crashes because of an exception that is thrown when logging something.

Therefore, my proposal would be some kind of implementation that looks like this:

public void LogDependency (HttpStatusCode statusCode, .... )
{
    LogDependency((int)statusCode, .... );
}

public void LogDependency (int statusCode, .... )
{
    Guard.NotLessThen(statusCode, 100);
    Guard.NotGreaterThen(statusCode, 599);

     ...
}

@fgheysels
Copy link
Member

When the PR is merged, I believe we should be able to create a new minor release.
This should allow @mic-hnk to use the functionality he contributed immediately in his project.

@stijnmoreels
Copy link
Member

When the PR is merged, I believe we should be able to create a new minor release. This should allow @mic-hnk to use the functionality he contributed immediately in his project.

Agreed! 👍

@stijnmoreels
Copy link
Member

Done in #515

@stijnmoreels
Copy link
Member

🎉 @mic-hnk, your feature is now available in Observability v2.8! Thx again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies All issues related to dependencies enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants