You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2020/10/27 22:47:21 UTC

[GitHub] [lucene-solr] saatchibhalla opened a new pull request #2040: SOLR-14965 add overseer queue size metrics

saatchibhalla opened a new pull request #2040:
URL: https://github.com/apache/lucene-solr/pull/2040


   <!--
   _(If you are a project committer then you may remove some/all of the following template.)_
   
   Before creating a pull request, please file an issue in the ASF Jira system for Lucene or Solr:
   
   * https://issues.apache.org/jira/projects/LUCENE
   * https://issues.apache.org/jira/projects/SOLR
   
   You will need to create an account in Jira in order to create an issue.
   
   The title of the PR should reference the Jira issue number in the form:
   
   * LUCENE-####: <short description of problem or changes>
   * SOLR-####: <short description of problem or changes>
   
   LUCENE and SOLR must be fully capitalized. A short description helps people scanning pull requests for items they can work on.
   
   Properly referencing the issue in the title ensures that Jira is correctly updated with code review comments and commits. -->
   
   
   # Description
   
   Please provide a short description of the changes you're making with this pull request.
   
   The Overseer work queues stored in ZK and abstracted by the Overseer can give us a good indication of the health of the cluster - if messages are taking too long to dequeue or the queue is growing too large, we know that the Overseer is overloaded and we are going to overwhelm the cluster.
   
   This work adds metrics to track the size of the Overseer queues (ClusterStateUpdate queue and Collections Work Queue).  
   
   # Solution
   
   Please provide a short description of the approach taken to implement your solution.
   
   Registered these two size metrics in a shared metrics registry for the Overseer. The Overseer shared metrics registry is only initialized upon reference so this shouldn't have any impact on metrics when run in non-cloud mode. Also updated the local solr-exporter-config.xml so that these metrics are exported and can be viewed locally in Grafana. 
   
   # Tests
   
   Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.
   
   Local testing was accomplished by sending CreateCollection load and monitoring a local grafana dashboard which accessed the metrics through prometheus-exporter. As the size of these queues change extremely quickly, it was difficult to see changes in these metrics quickly enough--and if the requests were sent too quickly we'd see local solr hosts OOMing. However, I was able to verify that the metrics are updating as the size of the queues change, and in a real environment with more sustained load we should see these metrics more accurately represent the state of the Overseer.
   
   Testing confirmed that we don't see the Overseer metrics registry in non-cloud mode.
   
   **/admin/metrics response containing added metrics**
   <img width="298" alt="Screen Shot 2020-10-27 at 5 56 48 PM" src="https://user-images.githubusercontent.com/16807693/97367278-32600980-187f-11eb-98b5-98a321c0f5c9.png">
   
   **local testing sending CreateCollection request load and seeing metrics update in real time**
   ![image](https://user-images.githubusercontent.com/16807693/97367388-5d4a5d80-187f-11eb-8c45-58abb0b0e0cc.png)
   
   
   
   # Checklist
   
   Please review the following and check all that apply:
   
   - [x] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
   - [x] I have created a Jira issue and added the issue ID to my pull request title.
   - [x] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
   - [x] I have developed this patch against the `master` branch.
   - [x] I have run `./gradlew check`.
   - [ ] I have added tests for my changes.
   - [x] I have added documentation for the [Ref Guide](https://github.com/apache/lucene-solr/tree/master/solr/solr-ref-guide) (for Solr changes only).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] saatchibhalla commented on pull request #2040: SOLR-14965 add overseer queue size metrics

Posted by GitBox <gi...@apache.org>.
saatchibhalla commented on pull request #2040:
URL: https://github.com/apache/lucene-solr/pull/2040#issuecomment-738939855


   Thanks @dsmiley! Looks good to me.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] dsmiley commented on pull request #2040: SOLR-14965 add overseer queue size metrics

Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #2040:
URL: https://github.com/apache/lucene-solr/pull/2040#issuecomment-738410528


   Looks good now Saatchi!  I'll merge tomorrow with the following CHANGES.txt note under "Improvements":
   
       SOLR-14965: metrics: Adds two metrics to the SolrCloud Overseer: solr_metrics_overseer_stateUpdateQueueSize
       and solr_metrics_overseer_collectionWorkQueueSize with corresponding entries in the the Prometheus exporter's
       default/stock configuration.  (Saatchi Bhalla, Megan Carey, Andrzej Białecki, David Smiley)
   
   @sigram if you prefer to or if you have lingering concerns, let me know.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] saatchibhalla commented on pull request #2040: SOLR-14965 add overseer queue size metrics

Posted by GitBox <gi...@apache.org>.
saatchibhalla commented on pull request #2040:
URL: https://github.com/apache/lucene-solr/pull/2040#issuecomment-722489672


   @sigram Would you mind taking another look at this PR whenever you have a chance? 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] megancarey commented on a change in pull request #2040: SOLR-14965 add overseer queue size metrics

Posted by GitBox <gi...@apache.org>.
megancarey commented on a change in pull request #2040:
URL: https://github.com/apache/lucene-solr/pull/2040#discussion_r513745248



##########
File path: solr/solr-ref-guide/src/metrics-reporting.adoc
##########
@@ -53,6 +53,12 @@ This registry is returned at `solr.jvm` and includes the following information.
 * System properties such as Java information, various installation directory paths, ports, and similar information. You can control what appears here by modifying `solr.xml`.
 // TODO for 7.0 fix this
 
+=== Overseer Registry

Review comment:
       We can update this doc comment now, since we are using the CoreContainer registry




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] sigram commented on a change in pull request #2040: SOLR-14965 add overseer queue size metrics

Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #2040:
URL: https://github.com/apache/lucene-solr/pull/2040#discussion_r513485514



##########
File path: solr/core/src/java/org/apache/solr/metrics/SolrMetricManager.java
##########
@@ -423,13 +429,13 @@ public boolean hasRegistry(String name) {
   /**
    * Check for predefined shared registry names. This compares the input name
    * with normalized names of predefined shared registries -
-   * {@link #JVM_REGISTRY} and {@link #JETTY_REGISTRY}.
+   * {@link #JVM_REGISTRY}, {@link #JETTY_REGISTRY}, and {@link #OVERSEER_REGISTRY}

Review comment:
       Overseer registry CANNOT be a shared registry because it's possible to have multiple Overseers in a single JVM, especially in unit tests. This should be a regular per-CoreContainer registry, which is created within the scope of the instance available from CC.getMetricManager().




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] sigram commented on a change in pull request #2040: SOLR-14965 add overseer queue size metrics

Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #2040:
URL: https://github.com/apache/lucene-solr/pull/2040#discussion_r513469553



##########
File path: solr/core/src/java/org/apache/solr/cloud/Overseer.java
##########
@@ -173,6 +175,9 @@
 
     private final Stats zkStats;
 
+    private final SolrMetricManager metricManager = new SolrMetricManager();

Review comment:
       This is wrong - you should never create instances of SolrMetricManager in your components. Always use the single instance constructed in `CoreContainer.getMetricManager()`.

##########
File path: solr/core/src/java/org/apache/solr/cloud/Overseer.java
##########
@@ -185,6 +190,9 @@ public ClusterStateUpdater(final ZkStateReader reader, final String myId, Stats
       this.completedMap = getCompletedMap(zkClient);
       this.myId = myId;
       this.reader = reader;
+
+      registryName = SolrMetricManager.getRegistryName(SolrInfoBean.Group.overseer);
+      metricManager.registerGauge(null, registryName, () -> stateUpdateQueue.getZkStats().getQueueLength(), "overseer", true, "stateUpdateQueueSize", "queue");

Review comment:
       You should create a `SolrMetricsContext` in Overseer, and create a child context here, and then pass it as the first argument to `registerGauge`. This helps to properly unregister old gauges from the registry (because a new instance of `ClusterStateUpdater` is created each time this Overseer instance becomes the leader, and then it registers a new instance of gauge with the same name). Using a static string "overseer" defeats this mechanism, because it falsely indicates that the life-cycle of `ClusterStateUpdater` is the same as its parent object Overseer.
   
   Then in the `ClusterStateUpdater.close()` method you should unregister this gauge using the value of `solrMetricsContext.getTag()`. This prevents obscure reference leaks in the metrics registry.

##########
File path: solr/core/src/java/org/apache/solr/cloud/OverseerTaskProcessor.java
##########
@@ -154,6 +158,9 @@ public OverseerTaskProcessor(ZkStateReader zkStateReader, String myId,
     this.runningTasks = ConcurrentHashMap.newKeySet();
     this.completedTasks = new ConcurrentHashMap<>();
     thisNode = Utils.getMDCNode();
+
+    registryName = SolrMetricManager.getRegistryName(SolrInfoBean.Group.overseer);
+    metricManager.registerGauge(null, registryName, () -> workQueue.getZkStats().getQueueLength(), "overseer", true, "collectionWorkQueueSize", "queue");

Review comment:
       See above comments - you should create a child context here (from the one created in Overseer) and use it in this call, and then unregister this gauge using this context's tag value.

##########
File path: solr/core/src/java/org/apache/solr/metrics/SolrMetricManager.java
##########
@@ -423,13 +429,13 @@ public boolean hasRegistry(String name) {
   /**
    * Check for predefined shared registry names. This compares the input name
    * with normalized names of predefined shared registries -
-   * {@link #JVM_REGISTRY} and {@link #JETTY_REGISTRY}.
+   * {@link #JVM_REGISTRY}, {@link #JETTY_REGISTRY}, and {@link #OVERSEER_REGISTRY}

Review comment:
       This is wrong - Overseer registry CANNOT be a shared registry because it's possible to have multiple Overseers in a single JVM, especially in unit tests. This should be a regular per-CoreContainer registry, which is created within the scope of the instance available from CC.getMetricManager().

##########
File path: solr/core/src/java/org/apache/solr/metrics/SolrMetricManager.java
##########
@@ -423,13 +429,13 @@ public boolean hasRegistry(String name) {
   /**
    * Check for predefined shared registry names. This compares the input name
    * with normalized names of predefined shared registries -
-   * {@link #JVM_REGISTRY} and {@link #JETTY_REGISTRY}.
+   * {@link #JVM_REGISTRY}, {@link #JETTY_REGISTRY}, and {@link #OVERSEER_REGISTRY}
    *
    * @param registry already normalized name
    * @return true if the name matches one of shared registries
    */
   private static boolean isSharedRegistry(String registry) {
-    return JETTY_REGISTRY.equals(registry) || JVM_REGISTRY.equals(registry);
+    return JETTY_REGISTRY.equals(registry) || JVM_REGISTRY.equals(registry) || OVERSEER_REGISTRY.equals(registry);

Review comment:
       See above - this can't be a shared registry.

##########
File path: solr/core/src/java/org/apache/solr/cloud/Overseer.java
##########
@@ -544,6 +552,7 @@ private LeaderStatus amILeader() {
     @Override
       public void close() {
         this.isClosed = true;
+        metricManager.unregisterGauges(registryName, "overseer");

Review comment:
       Please use the context tag value here.

##########
File path: solr/core/src/java/org/apache/solr/cloud/OverseerTaskProcessor.java
##########
@@ -386,6 +393,7 @@ private void cleanUpWorkQueue() throws KeeperException, InterruptedException {
 
   public void close() {
     isClosed = true;
+    metricManager.unregisterGauges(registryName, "overseer");

Review comment:
       See above - use the tag value from the context.

##########
File path: solr/core/src/java/org/apache/solr/cloud/OverseerTaskProcessor.java
##########
@@ -94,6 +96,8 @@
   private boolean isClosed;
 
   private volatile Stats stats;
+  private final SolrMetricManager metricManager = new SolrMetricManager();

Review comment:
       See above - this should never be done.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] saatchibhalla commented on pull request #2040: SOLR-14965 add overseer queue size metrics

Posted by GitBox <gi...@apache.org>.
saatchibhalla commented on pull request #2040:
URL: https://github.com/apache/lucene-solr/pull/2040#issuecomment-738118886


   > Saatchi; could you please comment here what this change in the config is about? Did you blend two metrics into one, and if so why? Maybe could you share a snippet of the prometheus exporter's output so we know clearly what the output of this "looks like"
   
   Hey David, sorry for the delay in getting back to you. I looked at this again, and decided that the two overseer queue metrics should be exported as separate metrics (rather than grouping them together under a single exported metric). Each of these queues represent separate responsibilities of the Overseer, and this allows for more flexibility in the future to add more metrics for each queue and group those together per queue. 
   
   Based on the current status of solr-exporter.xml this is how the exported metrics will look:
   {
     "name": "solr_metrics_overseer_stateUpdateQueueSize",
     "type": "GAUGE",
     "help": "See following URL: https://lucene.apache.org/solr/guide/metrics-reporting.html",
     "label_names": [],
     "label_values": [],
     "value": 0
   }
   {
     "name": "solr_metrics_overseer_collectionWorkQueueSize",
     "type": "GAUGE",
     "help": "See following URL: https://lucene.apache.org/solr/guide/metrics-reporting.html",
     "label_names": [],
     "label_values": [],
     "value": 0
   }


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] sigram commented on a change in pull request #2040: SOLR-14965 add overseer queue size metrics

Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #2040:
URL: https://github.com/apache/lucene-solr/pull/2040#discussion_r535413902



##########
File path: solr/core/src/java/org/apache/solr/cloud/Overseer.java
##########
@@ -185,6 +190,9 @@ public ClusterStateUpdater(final ZkStateReader reader, final String myId, Stats
       this.completedMap = getCompletedMap(zkClient);
       this.myId = myId;
       this.reader = reader;
+
+      registryName = SolrMetricManager.getRegistryName(SolrInfoBean.Group.overseer);
+      metricManager.registerGauge(null, registryName, () -> stateUpdateQueue.getZkStats().getQueueLength(), "overseer", true, "stateUpdateQueueSize", "queue");

Review comment:
       @megancarey unfortunately, there's not much documentation apart from the javadocs and user-level info in the RefGuide. We should create a doc in solr/dev-docs/metrics.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] saatchibhalla commented on a change in pull request #2040: SOLR-14965 add overseer queue size metrics

Posted by GitBox <gi...@apache.org>.
saatchibhalla commented on a change in pull request #2040:
URL: https://github.com/apache/lucene-solr/pull/2040#discussion_r513772342



##########
File path: solr/core/src/java/org/apache/solr/cloud/Overseer.java
##########
@@ -636,7 +650,7 @@ public synchronized void start(String id) {
     ThreadGroup ccTg = new ThreadGroup("Overseer collection creation process.");
 
     OverseerNodePrioritizer overseerPrioritizer = new OverseerNodePrioritizer(reader, getStateUpdateQueue(), adminPath, shardHandler.getShardHandlerFactory());
-    overseerCollectionConfigSetProcessor = new OverseerCollectionConfigSetProcessor(reader, id, shardHandler, adminPath, stats, Overseer.this, overseerPrioritizer);
+    overseerCollectionConfigSetProcessor = new OverseerCollectionConfigSetProcessor(reader, id, shardHandler, adminPath, stats, Overseer.this, overseerPrioritizer, solrMetricsContext);

Review comment:
       Updated the one test that calls this thanks for the catch!




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] sigram commented on a change in pull request #2040: SOLR-14965 add overseer queue size metrics

Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #2040:
URL: https://github.com/apache/lucene-solr/pull/2040#discussion_r513469553



##########
File path: solr/core/src/java/org/apache/solr/cloud/Overseer.java
##########
@@ -173,6 +175,9 @@
 
     private final Stats zkStats;
 
+    private final SolrMetricManager metricManager = new SolrMetricManager();

Review comment:
       You should never create instances of SolrMetricManager in your components. Always use the single instance constructed in `CoreContainer.getMetricManager()`.
   
   The only reason it kind of worked for you was that you made it a shared registry, which is created once per JVM - but as I explain below in this case a shared registry is incorrect and should not be used.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] saatchibhalla commented on a change in pull request #2040: SOLR-14965 add overseer queue size metrics

Posted by GitBox <gi...@apache.org>.
saatchibhalla commented on a change in pull request #2040:
URL: https://github.com/apache/lucene-solr/pull/2040#discussion_r513729917



##########
File path: solr/core/src/java/org/apache/solr/cloud/Overseer.java
##########
@@ -185,6 +190,9 @@ public ClusterStateUpdater(final ZkStateReader reader, final String myId, Stats
       this.completedMap = getCompletedMap(zkClient);
       this.myId = myId;
       this.reader = reader;
+
+      registryName = SolrMetricManager.getRegistryName(SolrInfoBean.Group.overseer);
+      metricManager.registerGauge(null, registryName, () -> stateUpdateQueue.getZkStats().getQueueLength(), "overseer", true, "stateUpdateQueueSize", "queue");

Review comment:
       @sigram Updated the PR with those proposed changes. Whenever you're free to take another look I'd appreciate it! 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] dsmiley merged pull request #2040: SOLR-14965 add overseer queue size metrics

Posted by GitBox <gi...@apache.org>.
dsmiley merged pull request #2040:
URL: https://github.com/apache/lucene-solr/pull/2040


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] sigram commented on a change in pull request #2040: SOLR-14965 add overseer queue size metrics

Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #2040:
URL: https://github.com/apache/lucene-solr/pull/2040#discussion_r513469553



##########
File path: solr/core/src/java/org/apache/solr/cloud/Overseer.java
##########
@@ -173,6 +175,9 @@
 
     private final Stats zkStats;
 
+    private final SolrMetricManager metricManager = new SolrMetricManager();

Review comment:
       This is wrong - you should never create instances of SolrMetricManager in your components. Always use the single instance constructed in `CoreContainer.getMetricManager()`.
   
   The only reason it kind of worked for you was that you made it a shared registry, which is created once per JVM - but as I explain below in this case a shared registry is incorrect and should not be used.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] saatchibhalla commented on a change in pull request #2040: SOLR-14965 add overseer queue size metrics

Posted by GitBox <gi...@apache.org>.
saatchibhalla commented on a change in pull request #2040:
URL: https://github.com/apache/lucene-solr/pull/2040#discussion_r513772094



##########
File path: solr/solr-ref-guide/src/metrics-reporting.adoc
##########
@@ -53,6 +53,12 @@ This registry is returned at `solr.jvm` and includes the following information.
 * System properties such as Java information, various installation directory paths, ports, and similar information. You can control what appears here by modifying `solr.xml`.
 // TODO for 7.0 fix this
 
+=== Overseer Registry

Review comment:
       chose to leave it this way - although we're using MetricManager from CoreContainer i'm still registering these metrics under "solr.overseer." 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] saatchibhalla commented on a change in pull request #2040: SOLR-14965 add overseer queue size metrics

Posted by GitBox <gi...@apache.org>.
saatchibhalla commented on a change in pull request #2040:
URL: https://github.com/apache/lucene-solr/pull/2040#discussion_r513612970



##########
File path: solr/core/src/java/org/apache/solr/cloud/Overseer.java
##########
@@ -185,6 +190,9 @@ public ClusterStateUpdater(final ZkStateReader reader, final String myId, Stats
       this.completedMap = getCompletedMap(zkClient);
       this.myId = myId;
       this.reader = reader;
+
+      registryName = SolrMetricManager.getRegistryName(SolrInfoBean.Group.overseer);
+      metricManager.registerGauge(null, registryName, () -> stateUpdateQueue.getZkStats().getQueueLength(), "overseer", true, "stateUpdateQueueSize", "queue");

Review comment:
       Thanks for the feedback, will update accordingly. 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] megancarey commented on a change in pull request #2040: SOLR-14965 add overseer queue size metrics

Posted by GitBox <gi...@apache.org>.
megancarey commented on a change in pull request #2040:
URL: https://github.com/apache/lucene-solr/pull/2040#discussion_r513632682



##########
File path: solr/core/src/java/org/apache/solr/cloud/Overseer.java
##########
@@ -185,6 +190,9 @@ public ClusterStateUpdater(final ZkStateReader reader, final String myId, Stats
       this.completedMap = getCompletedMap(zkClient);
       this.myId = myId;
       this.reader = reader;
+
+      registryName = SolrMetricManager.getRegistryName(SolrInfoBean.Group.overseer);
+      metricManager.registerGauge(null, registryName, () -> stateUpdateQueue.getZkStats().getQueueLength(), "overseer", true, "stateUpdateQueueSize", "queue");

Review comment:
       @sigram Thanks for the feedback! Is there documentation around adding metrics that covers things like this? 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] megancarey commented on a change in pull request #2040: SOLR-14965 add overseer queue size metrics

Posted by GitBox <gi...@apache.org>.
megancarey commented on a change in pull request #2040:
URL: https://github.com/apache/lucene-solr/pull/2040#discussion_r513744141



##########
File path: solr/core/src/java/org/apache/solr/cloud/Overseer.java
##########
@@ -636,7 +650,7 @@ public synchronized void start(String id) {
     ThreadGroup ccTg = new ThreadGroup("Overseer collection creation process.");
 
     OverseerNodePrioritizer overseerPrioritizer = new OverseerNodePrioritizer(reader, getStateUpdateQueue(), adminPath, shardHandler.getShardHandlerFactory());
-    overseerCollectionConfigSetProcessor = new OverseerCollectionConfigSetProcessor(reader, id, shardHandler, adminPath, stats, Overseer.this, overseerPrioritizer);
+    overseerCollectionConfigSetProcessor = new OverseerCollectionConfigSetProcessor(reader, id, shardHandler, adminPath, stats, Overseer.this, overseerPrioritizer, solrMetricsContext);

Review comment:
       Are there any tests that instantiate OverseerCollectionConfigSetProcessor? (Just want to make sure this is the only time this method is used, since we changed the signature)




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] megancarey commented on a change in pull request #2040: SOLR-14965 add overseer queue size metrics

Posted by GitBox <gi...@apache.org>.
megancarey commented on a change in pull request #2040:
URL: https://github.com/apache/lucene-solr/pull/2040#discussion_r513744685



##########
File path: solr/core/src/java/org/apache/solr/cloud/OverseerTaskProcessor.java
##########
@@ -134,13 +136,14 @@ public String toString() {
   private String thisNode;
 
   public OverseerTaskProcessor(ZkStateReader zkStateReader, String myId,
-                                        Stats stats,
-                                        OverseerMessageHandlerSelector selector,
-                                        OverseerNodePrioritizer prioritizer,
-                                        OverseerTaskQueue workQueue,
-                                        DistributedMap runningMap,
-                                        DistributedMap completedMap,
-                                        DistributedMap failureMap) {
+                               Stats stats,
+                               OverseerMessageHandlerSelector selector,
+                               OverseerNodePrioritizer prioritizer,
+                               OverseerTaskQueue workQueue,
+                               DistributedMap runningMap,
+                               DistributedMap completedMap,
+                               DistributedMap failureMap,
+                               SolrMetricsContext solrMetricsContext) {

Review comment:
       Same question here - would be good to double-check that this is the only place we instantiate OverseerTaskProcessor




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org