SOLR-14401: Track distrib/shard metrics differently by dsmiley · Pull Request #657 · apache/solr

dsmiley · 2022-02-18T23:31:24Z

only do for SearchHandler, not all request handlers
track all the same details at the shard level as request (more metrics)
don't limit this to SolrCloud

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

(I have some comments I'll post later)
(I have yet to updated affected tests; seems to be 1-2)

* only do for SearchHandler, not all request handlers * track all the same details at the shard level as request (more metrics) * don't limit this to SolrCloud

HoustonPutman

These changes look good to me. I don't have any particular opinion on the new naming, I see pros and cons, but if it works, it works.

Obviously we need some documentation around this, and I'm assuming that's coming next.

I think we need to change the default prometheus exporter config & graphana dashboard to use these new values, which will also give us a good indication if theres an issue with the way that the new naming is done.

madrob

Do we need to consolidate this with AuthenticationPlugin metrics as well?

solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java

dsmiley · 2022-02-23T05:50:33Z

Changes to the prometheus config are necessary. I want to follow some how-to on use of Grafana we have to ensure it looks right.
Also, performance-statistics-reference.adoc

dsmiley · 2022-02-23T05:51:41Z

Do we need to consolidate this with AuthenticationPlugin metrics as well?

Huh?

* needn't select ".distrib." as this is the default semantic * remove ".local." additions because these are already expressed via separate request handlers with a suffix * time_seconds_total computed differently; looks suspicious

dsmiley · 2022-03-03T14:04:30Z

Definitely see JIRA for my overall comments.

I want to point out that I observed that the "totalTime" metric has been a nanosecond number in recent years, yet once upon a time it was milliseconds. This change was very likely inadvertent. Our prometheus solr-exporter-config.xml shows that it thinks it's milliseconds. It's not; RequestHandlerBase increments this counter by "elapsed", the response of timer.stop() -- nanoseconds. Years ago it had invoked MetricUtils.nsToMs( but it appears @sigram removed this as a part of other changes in 2017 sometime -- d8df9f8

janhoy · 2022-03-03T14:59:25Z

Definitely see JIRA for my overall comments.

I want to point out that I observed that the "totalTime" metric has been a nanosecond number in recent years, yet once upon a time it was milliseconds. This change was very likely inadvertent. Our prometheus solr-exporter-config.xml shows that it thinks it's milliseconds. It's not; RequestHandlerBase increments this counter by "elapsed", the response of timer.stop() -- nanoseconds. Years ago it had invoked MetricUtils.nsToMs( but it appears @sigram removed this as a part of other changes in 2017 sometime -- d8df9f8

I have seen some very strange number in grafana dashboards for Solr with huge numbers. So I think this is a bug. Can you file a new JIRA for it?

dsmiley · 2022-03-03T15:10:29Z

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

…archHandler

…shard" or "false". Updated Grafana to use this to match former logic.

dsmiley · 2022-03-04T23:18:03Z

Ready for review again. Pending:

updated CHANGES.txt, major-changes-in-solr-9.adoc
precommit fix / merge with main
updating the Solr ref guide

# Conflicts: # solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java # solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java # solr/core/src/java/org/apache/solr/util/SolrPluginUtils.java

dsmiley · 2022-03-08T20:52:40Z

I think it's ready finally. Please check out the ref guide explanation that I just pushed.

I think the outdated MetricsQueryTemplateTest ought to be replaced with a very different integration test that is more of a sanity check that certain metrics we expect to see are present. Such a test could be done via docker and it'd thus offer more/better test coverage that other aspects are working too.

HoustonPutman

This looks good to me, but I haven't gone through and tested it yet. Might get time to do that tomorrow

HoustonPutman · 2022-03-09T22:41:36Z

solr/prometheus-exporter/build.gradle

  main = project.ext.mainClass
  classpath = sourceSets.main.runtimeClasspath
  systemProperties = ["log4j.configurationFile":"file:conf/log4j2.xml"]
+  args = ["-f", "conf/solr-exporter-config.xml"]


Ahh we could also add this to the classpath above like we do in the bin script.

Though it's not something we need to do in this PR.

HoustonPutman · 2022-03-10T18:31:30Z

Ok found an issue, the core-query template isn't the only affected template for prometheus. core has metrics that contain [shard] as well, including (but not limited to):

solr_metrics_core_errors_total
solr_metrics_core_requests_total
solr_metrics_core_timeouts_total

It was an easy fix, adding the same code from the core-query template, but it also means that the internal=false tag will be added for other handlers that may be used internally, we just don't include the tag for it. Personally I think that's fine and it can be seen as a possible improvement for the future. (I don't think the graphana dashboard is necessarily impacted by this. We could certainly add the tag filter for these metrics, but I don't think its necessary for this PR)

I'll go ahead and push the fix and let you take a look @dsmiley. Otherwise I'm good with this. Feel free to revert and fix in another way if you prefer.

dsmiley · 2022-03-10T22:42:36Z

Thanks Houston! Yeah I wondered about the other templates but I didn't notice that they wold process such cases.

* only do for SearchHandler, not all request handlers (less metrics overall) * track all the same details at the shard level as request (more detailed metrics) * use [shard] suffix; do away with .distrib. and .local. * don't limit this to SolrCloud Prometheus Exporter & Grafana config: * remove select ".distrib."; this is the default semantic * remove ".local." additions because these are already expressed via separate request handlers with a suffix * time_seconds_total computed differently; looks suspicious * extract an "internal" Prometheus label from the handler; has values "shard" or "false". Updated Grafana to use this to match former logic. Misc: * prometheus gradle: fix "run" task * fix README link Co-authored-by: Houston Putman <houston@apache.org>

SOLR-14401: Track distrib/shard metrics differently

89498d1

* only do for SearchHandler, not all request handlers * track all the same details at the shard level as request (more metrics) * don't limit this to SolrCloud

dsmiley requested a review from HoustonPutman February 18, 2022 23:31

HoustonPutman reviewed Feb 21, 2022

View reviewed changes

madrob reviewed Feb 21, 2022

View reviewed changes

solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java Outdated Show resolved Hide resolved

solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java Outdated Show resolved Hide resolved

dsmiley added 3 commits February 22, 2022 19:30

RhMetrics -> HandlerMetrics

c8f0e83

Merge branch 'main' into solr-14401-distribMetrics

196e071

Fix test

22be008

dsmiley added 5 commits March 3, 2022 08:25

rename

e77523b

exporter config:

cc18efe

* needn't select ".distrib." as this is the default semantic * remove ".local." additions because these are already expressed via separate request handlers with a suffix * time_seconds_total computed differently; looks suspicious

prometheus gradle: fix "run" task

6ad903b

fix README link

4363e82

CHANGES.txt

e9a5682

dsmiley added 4 commits March 4, 2022 11:51

Move all shard metrics handling out of RequestHandlerBase and into Se…

b0efcc5

…archHandler

Extract an "internal" Prometheus label from the handler; has values "…

6f06783

…shard" or "false". Updated Grafana to use this to match former logic.

remove accidentally added grafana config file

7d6e86b

Updated grafana-solr-dashboard.json to use internal label

e8c768e

dsmiley added 4 commits March 4, 2022 18:19

CHANGES.txt

9ab6002

unused import

af9f066

Merge branch 'main' into solr-14401-distribMetrics

92a2392

# Conflicts: # solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java # solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java # solr/core/src/java/org/apache/solr/util/SolrPluginUtils.java

ref guide

bf903c2

HoustonPutman reviewed Mar 9, 2022

View reviewed changes

Add internal tag for core metrics

caba3de

Merge branch 'main' into solr-14401-distribMetrics

51ee02b

dsmiley merged commit a8ae853 into apache:main Mar 10, 2022

dsmiley deleted the solr-14401-distribMetrics branch March 10, 2022 22:41

Comments

Conversation

dsmiley commented Feb 18, 2022

Uh oh!

HoustonPutman left a comment

Choose a reason for hiding this comment

Uh oh!

madrob left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dsmiley commented Feb 23, 2022

Uh oh!

dsmiley commented Feb 23, 2022

Uh oh!

dsmiley commented Mar 3, 2022

Uh oh!

janhoy commented Mar 3, 2022

Uh oh!

dsmiley commented Mar 3, 2022

Uh oh!

dsmiley commented Mar 4, 2022

Uh oh!

dsmiley commented Mar 8, 2022

Uh oh!

HoustonPutman left a comment

Choose a reason for hiding this comment

Uh oh!

HoustonPutman Mar 9, 2022

Choose a reason for hiding this comment

Uh oh!

HoustonPutman commented Mar 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dsmiley commented Mar 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

HoustonPutman commented Mar 10, 2022 •

edited

Loading