openzipkin / zipkin4net

A .NET client library for Zipkin
Apache License 2.0
342 stars 87 forks source link

Consistent error practice for http #113

Open codefromthecrypt opened 7 years ago

codefromthecrypt commented 7 years ago

Now that zipkin-dependencies adds errorCount, it might help to see what others are doing for "error". Notably, I think here we are already adding error on an exception. What might not be happening here is an error for http error status. Here's logic we use in Brave, which might be helpful to see if things are consistent or not.

    String message = null;
    if (error != null) {
      message = error.getMessage();
      if (message == null) message = error.getClass().getSimpleName();
    } else if (httpStatus != null){
      message = httpStatus < 200 || httpStatus > 399 ? String.valueOf(httpStatus) : null;
    }
    if (message != null) customizer.tag(Constants.ERROR, message);

Note: the above is just a default

crntn commented 7 years ago

I can confirm that currently there is no error added on an http error status. I think this issue is part of a bigger story to give more flexibility to users to override default behaviors without having to re-code part of the middleware.

codefromthecrypt commented 7 years ago

I think this issue is part of a bigger story to give more flexibility to users to override default behaviors without having to re-code part of the middleware.

I'd suggest it isn't :) error code defaults help with dependency links as otherwise http 500 will not increment error count. Most in brave for example don't override this eventhough they can. making non-success error by default is a.. good default.

Separately, agree we can work on customization, but I'd suggest this is a good default regardless of that.

crntn commented 7 years ago

Sounds good to me !