SOLR-18400: Plugins screen 500s when metrics are disabled

[janhoy]

https://issues.apache.org/jira/browse/SOLR-18400

When metrics collection is disabled, return HTTP 510 INVALID STATE, instead of throwing IOException("No metrics found in response")` which leads to → HTTP 500 and a blank Plugins screen.

[janhoy]

Note: the v2 metrics API (GetMetrics) still throws a 510 INVALID_STATE error when metrics collection is disabled, so v1 and v2 now behave differently (v1 returns 200 with a # metrics collection is disabled comment). I left v2 as-is since this PR targets the v1 endpoint the Admin UI uses, but we may want to align v2 with the same graceful behavior in a follow-up.

[epugh]

Note: the v2 metrics API (GetMetrics) still throws a 510 INVALID_STATE error when metrics collection is disabled, so v1 and v2 now behave differently (v1 returns 200 with a # metrics collection is disabled comment). I left v2 as-is since this PR targets the v1 endpoint the Admin UI uses, but we may want to align v2 with the same graceful behavior in a follow-up.

It would make life easier when we move if V2 did the same as V1... One reason I'm axinous to get us to V2 everywhere we can is that we've seen this pattern of fixes making it to V1 when we also have V2, and then it falls behind...

[epugh]

@janhoy I think this was very close, so I updated it from main. I am also responding to a copilot item!

One question, are you suggesting that the 501 is a nicer way of handling this situation versus the current string matching approach? If so, I am also wondering if you are suggesting that we look at lots of apis through the lens of "this is a good time to throw a 501 error" and maybe make the a consistent pattern in our V2 apis? Or am I reading too much into this.

Lastly, I wonder how far our V2 GetMetrics is from being usable int eh admin ui instead of hte v1?

Backend (v1 endpoint):

curl http://localhost:8987/solr/admin/metrics?wt=prometheus
→ HTTP 200
# metrics collection is disabled
# EOF

v2 endpoint (unfixed, confirms janhoy's comment):

curl -H "Accept: text/plain; version=0.0.4" http://localhost:8987/api/metrics
→ HTTP 510, {"error":{"code":510,"msg":"Metrics collection is disabled"}}

[epugh]

LGTM!

[janhoy]

The 510 code used by v2 looks a bit off, although I understand the intent behind it, it is better than a 503 which could signal that the entire Solr node is unavailable.

Prometheus scraping an endpoint will interpret any non-200 code as "solr scrape target unavailable" and flag it as DOWN. A 200-code with empty response will be interpreted as "solr scrape target functioning, but no data" and flag it as "UP".

You can argue both ways. But agree v1 and v2 shuold probably be aligned. I'm not opposed to instead align v1 with HTTP 510 and adapt frontend to detect this as a "metrics disabled" signal instead of text parsing?

[janhoy]

I think I like HTTP 510 better in general, so I vote for having both v1 and v2 APIs err with HTTP 510 when metrics are disabled. And adapt frontend to cope.

[Copilot]

Copilot review overview

🟡 Changes recommended

Restrict global HTTP 510 suppression to the metrics request and add Admin UI coverage for the disabled-metrics path.

Review effort: Lite
Findings: 1 · 1

Open (2)

• Restrict 510 error suppression to metrics requests · New
• Add Admin UI test coverage for metrics-disabled 510 responses · New

Resolved since last review (2)

• OpenMetrics requests also reach this branch (writeOpenMetricsFormat is selected by…
• metricsEnabled is not read by the shipped server/solr/solr.xml; the enabled value is taken from…

[epugh]

Interestingly I was just poiking at the SQL UI in the admin tool, if you don't enable the module you get an error when you run a sql query. I modified it to catch the classcast exception "no SQLHandler found" and then show a nice message about enabling the module. I first thought hoguht "hey, can I consult which plugins/endpoints are avialable via v2 apis" and was going to see if the /sql existed or not. BUt that API doesn't exist. SO I went with the narrower fix.

I wonder if we should return 510 instead of a classcast exception if you hit /sql and it can't load?

[epugh]

LGTM.