Re: [PR] SOLR-17857: Ensure SolrMetricsContext is closed [solr]

2026-03-28 Thread via GitHub 


dsmiley merged PR #4226:
URL: https://github.com/apache/solr/pull/4226


-- 
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.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] SOLR-17857: Ensure SolrMetricsContext is closed [solr]

2026-03-26 Thread via GitHub 


dsmiley commented on PR #4226:
URL: https://github.com/apache/solr/pull/4226#issuecomment-4139896222

   Assuming tests pass etc., I plan to merge Saturday.
   It's slightly tempting to leave `unregister` as an alias for close but... 
this is internal.
   
   Commit message:
   
   SolrMetricsContext lifecycle management  


 - Made SolrMetricsContext implement AutoCloseable and renamed unregister() 
→ close()
 - Added ObjectReleaseTracker integration for leak detection
 - Updated 30+ components to properly close their metrics contexts (caches, 
handlers, security plugins, update components, etc.)
 - Fixed double-close issues causing OTEL warnings
 - Added assertions in RequestHandlerBase to enforce metrics initialization 
before request handling
   


-- 
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.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] SOLR-17857: Ensure SolrMetricsContext is closed [solr]

2026-03-23 Thread via GitHub 


HoustonPutman commented on code in PR #4226:
URL: https://github.com/apache/solr/pull/4226#discussion_r2976341206


##
solr/core/src/java/org/apache/solr/handler/admin/CoreAdminHandler.java:
##
@@ -139,7 +139,7 @@ public void initializeMetrics(SolrMetricsContext 
parentContext, Attributes attri
 coreAdminAsyncTracker.expensiveExecutor =
 solrMetricsContext.instrumentedExecutorService(
 coreAdminAsyncTracker.expensiveExecutor,
-"solr_node_executor",
+"solr_node_expensive_executor",

Review Comment:
   ahhh ok, yeah I was going off of the logs of double-closed instruments and I 
don't think they had the `name` information there. We should revert if this 
actually isn't an issue.



-- 
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.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] SOLR-17857: Ensure SolrMetricsContext is closed [solr]

2026-03-23 Thread via GitHub 


mlbiscoc commented on code in PR #4226:
URL: https://github.com/apache/solr/pull/4226#discussion_r2975819899


##
solr/core/src/java/org/apache/solr/handler/admin/CoreAdminHandler.java:
##
@@ -139,7 +139,7 @@ public void initializeMetrics(SolrMetricsContext 
parentContext, Attributes attri
 coreAdminAsyncTracker.expensiveExecutor =
 solrMetricsContext.instrumentedExecutorService(
 coreAdminAsyncTracker.expensiveExecutor,
-"solr_node_executor",
+"solr_node_expensive_executor",

Review Comment:
   I wouldn't say this is right or wrong I guess just preference how we want to 
show this. `solr_node_executor` is just the prefix but the executor has 
attribute `name` to that actually groups what executor this metric was tracking.
   
   ```
   # HELP solr_node_executor_thread_pool_tasks Thread pool task counts
   # TYPE solr_node_executor_thread_pool_tasks gauge
   
solr_node_executor_thread_pool_tasks{category="ADMIN",name="asyncCoreAdminExecutor",otel_scope_name="org.apache.solr",type="active"}
 0.0
   
solr_node_executor_thread_pool_tasks{category="ADMIN",name="asyncCoreExpensiveAdminExecutor",otel_scope_name="org.apache.solr",type="active"}
 0.0
   ```
   
   to now
   
   ```
   # HELP solr_node_executor_thread_pool_tasks Thread pool task counts
   # TYPE solr_node_executor_thread_pool_tasks gauge
   
solr_node_executor_thread_pool_tasks{category="ADMIN",name="asyncCoreAdminExecutor",otel_scope_name="org.apache.solr",type="active"}
 0.0
   
   # HELP solr_node_executor_thread_pool_tasks Thread pool task counts
   # TYPE solr_node_expensive_executor_thread_pool_tasks gauge
   
solr_node_expensive_executor_thread_pool_tasks{category="ADMIN",name="asyncCoreExpensiveAdminExecutor",otel_scope_name="org.apache.solr",type="active"}
 0.0
   ```
   
   We now create 2 separate metrics. I personally liked it how it was before 
since it was grouped and differentiated by name. I'm ok though if you both 
disagree and prefer the differentiator in the name.
   
   



-- 
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.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] SOLR-17857: Ensure SolrMetricsContext is closed [solr]

2026-03-23 Thread via GitHub 


mlbiscoc commented on code in PR #4226:
URL: https://github.com/apache/solr/pull/4226#discussion_r2975819899


##
solr/core/src/java/org/apache/solr/handler/admin/CoreAdminHandler.java:
##
@@ -139,7 +139,7 @@ public void initializeMetrics(SolrMetricsContext 
parentContext, Attributes attri
 coreAdminAsyncTracker.expensiveExecutor =
 solrMetricsContext.instrumentedExecutorService(
 coreAdminAsyncTracker.expensiveExecutor,
-"solr_node_executor",
+"solr_node_expensive_executor",

Review Comment:
   I wouldn't say this is right or wrong I guess just preference how we want to 
show this. `solr_node_executor` is just the prefix but the executor has 
attribute `name` to that actually groups what executor this metric was tracking.
   
   ```
   # HELP solr_node_executor_thread_pool_tasks Thread pool task counts
   # TYPE solr_node_executor_thread_pool_tasks gauge
   
solr_node_executor_thread_pool_tasks{category="ADMIN",name="asyncCoreAdminExecutor",otel_scope_name="org.apache.solr",type="active"}
 0.0
   
solr_node_executor_thread_pool_tasks{category="ADMIN",name="asyncCoreExpensiveAdminExecutor",otel_scope_name="org.apache.solr",type="active"}
 0.0
   ```
   
   to now
   
   ```
   # HELP solr_node_executor_thread_pool_tasks Thread pool task counts
   # TYPE solr_node_executor_thread_pool_tasks gauge
   
solr_node_executor_thread_pool_tasks{category="ADMIN",name="asyncCoreAdminExecutor",otel_scope_name="org.apache.solr",type="active"}
 0.0
   
   # HELP solr_node_executor_thread_pool_tasks Thread pool task counts
   # TYPE solr_node_expensive_executor_thread_pool_tasks gauge
   
solr_node_expensive_executor_thread_pool_tasks{category="ADMIN",name="asyncCoreExpensiveAdminExecutor",otel_scope_name="org.apache.solr",type="active"}
 0.0
   ```
   
   We now create 2 separate metrics. I personally liked it how it was before 
since it was grouped and differentiated by name.
   
   



-- 
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.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]