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 CircuitBreaker timesTripped metric #203

Merged
merged 7 commits into from
May 31, 2024

Conversation

nginthfs
Copy link
Collaborator

@nginthfs nginthfs commented May 29, 2024

This PR adds a metric for each CircuitBreaker that is configured in Solr. The metrics are named like so:

# times_tripped_<name of circuit breaker class>_<request type>
times_tripped_load_average_circuit_breaker_query

All metrics from every CircuitBreaker of the same class and request type are rolled up into this one metric, and reported through the PrometheusMetricsServlet.

Testing

Tested by adding this to a config on my local machine:

...
  <circuitBreaker class="solr.LoadAverageCircuitBreaker">
    <double name="threshold">1</double>
    <arr name="requestTypes">
      <str>update</str>
    </arr>
  </circuitBreaker>

  <circuitBreaker class="solr.LoadAverageCircuitBreaker">
    <double name="threshold">1</double>
    <arr name="requestTypes">
      <str>query</str>
    </arr>
  </circuitBreaker>
...

Then, issuing some requests to a collection, and seeing the circuit breaker activate. Then, I checked http://localhost:8983/solr/metrics to make sure this showed up:

# HELP times_tripped_load_average_circuit_breaker_query number of times circuit has been tripped
# TYPE times_tripped_load_average_circuit_breaker_query counter
times_tripped_load_average_circuit_breaker_query 1
# HELP times_tripped_load_average_circuit_breaker_update number of times circuit has been tripped
# TYPE times_tripped_load_average_circuit_breaker_update counter
times_tripped_load_average_circuit_breaker_update 0

Also tested that Solr functions as normal when no CircuitBreakers are configured.

@nginthfs
Copy link
Collaborator Author

Reworking this a bit, will update when ready for review again.

@nginthfs
Copy link
Collaborator Author

Okay this is ready for review again!

Copy link
Collaborator

@hiteshk25 hiteshk25 left a comment

Choose a reason for hiding this comment

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

LGTM

@hiteshk25
Copy link
Collaborator

QQ: it does add node name as label some where?

@nginthfs
Copy link
Collaborator Author

QQ: it does add node name as label some where?

I believe all Solr metrics have nodename added to them automatically as a label later on in the pipeline. For example, metrics_qtime, which is configured in a similar way in PrometheusMetricsServlet has nodename as a label if you look at it in Grafana. I'm pretty sure this happens in the Prometheus k8s deployment.

@nginthfs
Copy link
Collaborator Author

QQ: it does add node name as label some where?

I believe all Solr metrics have nodename added to them automatically as a label later on in the pipeline. For example, metrics_qtime, which is configured in a similar way in PrometheusMetricsServlet has nodename as a label if you look at it in Grafana. I'm pretty sure this happens in the Prometheus k8s deployment.

Confirmed this by deploying to playpen.

@nginthfs nginthfs merged commit 9302dcf into fs/branch_9_3 May 31, 2024
7 checks passed
magibney pushed a commit that referenced this pull request Oct 10, 2024
* add CircuitBreaker timesTripped metric

* remove String.format

* toLowerCase add locale

* tidy

* move metrics management to its own map

* use stdlib Map.copyOf

* tidy

(cherry picked from commit 9302dcf)
magibney pushed a commit that referenced this pull request Oct 10, 2024
* add CircuitBreaker timesTripped metric

* remove String.format

* toLowerCase add locale

* tidy

* move metrics management to its own map

* use stdlib Map.copyOf

* tidy

(cherry picked from commit 9302dcf)
magibney pushed a commit that referenced this pull request Oct 11, 2024
* add CircuitBreaker timesTripped metric

* remove String.format

* toLowerCase add locale

* tidy

* move metrics management to its own map

* use stdlib Map.copyOf

* tidy

(cherry picked from commit 9302dcf)
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