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

Requests that end up with a HystrixBadRequestException are not visible in Dashboard traffic #186

Closed
pparth opened this issue Oct 1, 2013 · 12 comments

Comments

@pparth
Copy link

pparth commented Oct 1, 2013

Hello,

I used HystrixBadRequestException in order to differentiate between the 4XX http errors and the 5XX ones, when wrapping an http client using a Hystrix command. I can see in the Dashboard that these requests do not count as Exceptions. But to my surprise, i also see that they do not count at all! So, we have this weird thing where the requests are properly monitored in the Pool traffic, but there is not traffic in the relevant Command. If we accept that these are client-side errors, then i would assume that they should be countered as Succeeded. Is this a bug or maybe i am missing something important in the whole rationale?

@benjchristensen
Copy link
Contributor

Interesting, that would indeed be a bug.

I agree it could count as a success but I'm concerned it will confuse people. On the flip side, having another metric to track and display is not desirable.

Does anyone else have an opinion?

Shall I proceed to make these requests count against the 'Success' metric?

@pparth
Copy link
Author

pparth commented Oct 1, 2013

Well, IMHO, confusion is avoided only if we add another "greeny-like" metric to track. As this out of the question due to the fixes that need to be done in the stream structure and dashboard, i think that not measuring these requests is a little more confusing than adding them to Success.

@pparth
Copy link
Author

pparth commented Oct 1, 2013

Or at least, not add them to Success metric but add them to request rates and latency percentiles as traffic.

@benjchristensen
Copy link
Contributor

The request rates are derived from Success+Timeout+ other metrics, so there isn't another top-level counter.

I don't like the idea of another "green" metric.

I'm leaning towards counting them as 'Success' but also adding another metric for detailed inspection, similar to how countFallbacks and countExceptionsThrown exist, but aren't used to represent traffic numbers.

@daveray and @abersnaze What do you think should be done here?

@daveray
Copy link
Contributor

daveray commented Oct 2, 2013

I like making it a separate detail metric. I'm already confused enough when I look at the dashboard without another green thing there.

@benjchristensen
Copy link
Contributor

I agree with the sentiment, but that wouldn't solve the issue of requests not showing up in the overall rate of traffic. In other words, the RPS could be 0 but there could actually be BadRequest traffic through it without having this metric involved in the main display.

@pparth
Copy link
Author

pparth commented Nov 13, 2013

Anything new on that?

@benjchristensen
Copy link
Contributor

Over a year old ... putting in 1.4.x in case anyone wants to pick this up.

@benjchristensen benjchristensen added this to the 1.4.x milestone Dec 12, 2014
@mattrjacobs mattrjacobs removed this from the 1.4.x milestone Dec 19, 2014
@pparth pparth removed this from the 1.4.x milestone Dec 19, 2014
@mattrjacobs
Copy link
Contributor

#13 consider creating a separate metric for HystrixBadRequestExceptions. I will consider this issue to track the consumption of this metric, so will therefore mark it in the Dashboard milestone

@mattrjacobs mattrjacobs added this to the Dashboard 1.1 milestone Dec 19, 2014
@mattrjacobs
Copy link
Contributor

I just added a metric to track HystrixBadRequestExceptions. If anyone is interested in adding it to the dashboard, please submit a PR.

@mattrjacobs mattrjacobs modified the milestones: Dashboard 1.1, 1.4.1 Mar 3, 2015
@mattrjacobs
Copy link
Contributor

The change in #776 adds the existing bad request metric to the dashboard (in the lower left-hand corner with the color of lightSeaGreen). No semantics of command operation were touched

hugowetterberg added a commit to hugowetterberg/hystrix-go that referenced this issue Jun 24, 2015
I just tried to use the Hystrix Dashboard and it broke because of a missing `rollingCountBadRequests` property.

This adds that counter, and the required stuff around it.

What I'm unsure about here is how a bad request should be tracked, it's not very well documented in Hystrix itself (...or, I just couldn't find the docs). I found this issue: Netflix/Hystrix#186 that discussed bad requests in relationship to the dash.

BREAKING CHANGE: This is a breaking change for anyone implementing custom collectors as the collector interface has a new function `IncrementBadRequests()`.
@yoorick
Copy link

yoorick commented Nov 1, 2017

Hello,

I have a question, why the added badRequests counter still doesn't count towards totalRequestCount.
Was this an intentional decision? If yes, what's the reason behind that?

Today, in case of any significant amount of Bad Requests, you get misleading numbers in the metrics stream like that:

{
  "errorPercentage": 50,
  "errorCount": 16,
  "requestCount": 32,
  "rollingCountBadRequests": 30,
  "rollingCountExceptionsThrown": 30,
  "rollingCountFailure": 16,
  "rollingCountSuccess": 16,
  ...
}

You can see that real request count in this example is 62. (And real errorPercentage is either ~25% or ~75%, depending on how to treat bad requests, but not 50%).

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

No branches or pull requests

5 participants