Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 6.4, 7.0
    • Component/s: metrics
    • Security Level: Public (Default Security Level. Issues are Public)
    • Labels:
      None

      Description

      Misc cleanup in metrics API to fix:

      1. metrics reporting themselves under the wrong category
      2. core container metrics are without a category
      1. screenshot-1.png
        449 kB
        Andrzej Bialecki
      2. SOLR-9947.patch
        61 kB
        Andrzej Bialecki
      3. SOLR-9947.patch
        35 kB
        Andrzej Bialecki
      4. SOLR-9947.patch
        27 kB
        Andrzej Bialecki
      5. SOLR-9947-with-mbeans.patch
        61 kB
        Andrzej Bialecki

        Issue Links

          Activity

          Hide
          shalinmangar Shalin Shekhar Mangar added a comment -

          A few of the things to fix:

          1. Config handler is under "OTHERS" category
          2. The /update/* handlers are under "QUERYHANDLER"
          3. The container level metrics such as "core.*" gauges, threadPool.coreContainerWorkExecutor and threadPool.coreLoadExecutor are without a category unlike other metrics. Perhaps we can introduce a CONTAINER category?
          4. InfoHandler which is created in CoreContainer and has sub-handler such as /admin/logging, /admin/threads etc which show up in each core. I am not sure why.

          I think we are too tied with SolrInfoMBean and the categories that exist already. It feels wrong for a new API to use outdated category names. For example, "/replication" is not a query handler, it is something else entirely. Same for core admin handlers and other such stuff. I am not sure how to fix this without breaking back-compat with existing monitoring solutions. What do you think Andrzej Bialecki ?

          Show
          shalinmangar Shalin Shekhar Mangar added a comment - A few of the things to fix: Config handler is under "OTHERS" category The /update/* handlers are under "QUERYHANDLER" The container level metrics such as "core.*" gauges, threadPool.coreContainerWorkExecutor and threadPool.coreLoadExecutor are without a category unlike other metrics. Perhaps we can introduce a CONTAINER category? InfoHandler which is created in CoreContainer and has sub-handler such as /admin/logging, /admin/threads etc which show up in each core. I am not sure why. I think we are too tied with SolrInfoMBean and the categories that exist already. It feels wrong for a new API to use outdated category names. For example, "/replication" is not a query handler, it is something else entirely. Same for core admin handlers and other such stuff. I am not sure how to fix this without breaking back-compat with existing monitoring solutions. What do you think Andrzej Bialecki ?
          Hide
          ab Andrzej Bialecki added a comment -

          We can put "infrastructure" handlers into other categories, especially those that weren't previously reported via JMX anyway.

          Moving SolrConfigHandler from OTHER would slightly break back-compat, since this value is also reported in the old JMX interface as a bean attribute, similarly moving /update or /replication handlers from QUERYHANDLER. I'm not sure whether it's a big deal or not - maybe Otis Gospodnetic or Walter Underwood could comment on this?

          Show
          ab Andrzej Bialecki added a comment - We can put "infrastructure" handlers into other categories, especially those that weren't previously reported via JMX anyway. Moving SolrConfigHandler from OTHER would slightly break back-compat, since this value is also reported in the old JMX interface as a bean attribute, similarly moving /update or /replication handlers from QUERYHANDLER. I'm not sure whether it's a big deal or not - maybe Otis Gospodnetic or Walter Underwood could comment on this?
          Hide
          ab Andrzej Bialecki added a comment -

          Current patch with the following changes:

          • simplified UPDATEHANDLER and QUERYHANDLER to UPDATE and QUERY respectively. This also allows us to put into this category those plugins that are not handlers.
          • added ADMIN and CONTAINER. The first category covers all admin handlers, the other covers CoreContainer-specific handlers.
          • moved some handlers to UPDATE and REPLICATION

          This seems to bring much more consistency and transparency into where plugins logically belong to, as oppose to them being RequestHandlerBase subclasses, which is an implementation detail. If there are no objections I'd like to commit it soon, so that it makes it into 6.4.

          Show
          ab Andrzej Bialecki added a comment - Current patch with the following changes: simplified UPDATEHANDLER and QUERYHANDLER to UPDATE and QUERY respectively. This also allows us to put into this category those plugins that are not handlers. added ADMIN and CONTAINER. The first category covers all admin handlers, the other covers CoreContainer-specific handlers. moved some handlers to UPDATE and REPLICATION This seems to bring much more consistency and transparency into where plugins logically belong to, as oppose to them being RequestHandlerBase subclasses, which is an implementation detail. If there are no objections I'd like to commit it soon, so that it makes it into 6.4.
          Hide
          dsmiley David Smiley added a comment -

          The existing SolrInfoMBeans hierarchy isn't what we want them to be if we were to start over. It's good to see a refactor like this. Is it possible to register beans in the legacy location and the new location as a transition? And then in the UI we'd not show the legacy ones.

          Show
          dsmiley David Smiley added a comment - The existing SolrInfoMBeans hierarchy isn't what we want them to be if we were to start over. It's good to see a refactor like this. Is it possible to register beans in the legacy location and the new location as a transition? And then in the UI we'd not show the legacy ones.
          Hide
          ab Andrzej Bialecki added a comment -

          These changes only marginally affect the legacy mbeans, in that their "category" attribute may be different than before but their hierarchy (such as it is) remains unchanged.

          Or do you mean that we should register them both at the legacy and new locations? I can see that could be useful - SolrInfoMBean interface provides many useful non-numeric data, which the new metrics API doesn't cover at all. Let me see how much work it would take, maybe this should be done in a separate issue.

          Show
          ab Andrzej Bialecki added a comment - These changes only marginally affect the legacy mbeans, in that their "category" attribute may be different than before but their hierarchy (such as it is) remains unchanged. Or do you mean that we should register them both at the legacy and new locations? I can see that could be useful - SolrInfoMBean interface provides many useful non-numeric data, which the new metrics API doesn't cover at all. Let me see how much work it would take, maybe this should be done in a separate issue.
          Hide
          ab Andrzej Bialecki added a comment -

          More cleanups:

          • fix broken tests
          • always report total size of tlogs, not just during recovery
          • add a note about UpdateLog.State enum being reported, to take care when adding or modifying this enum, and use explicit values for this enum.
          • HIGHTLIGHTING -> HIGHLIGHTER for consistency
          • TLOG.applying_buffered.ops -> TLOG.applyingBuffered.ops for consistency.
          Show
          ab Andrzej Bialecki added a comment - More cleanups: fix broken tests always report total size of tlogs, not just during recovery add a note about UpdateLog.State enum being reported, to take care when adding or modifying this enum, and use explicit values for this enum. HIGHTLIGHTING -> HIGHLIGHTER for consistency TLOG.applying_buffered.ops -> TLOG.applyingBuffered.ops for consistency.
          Hide
          dsmiley David Smiley added a comment -

          Or do you mean that we should register them both at the legacy and new locations?

          Yes that's what I mean.

          Show
          dsmiley David Smiley added a comment - Or do you mean that we should register them both at the legacy and new locations? Yes that's what I mean.
          Hide
          ab Andrzej Bialecki added a comment - - edited

          It turned out that it's not a major change - I attached a screenshot of what it looks like in JConsole.
          Edit: the "_jmx_NNNNNN" is a reporter name in the new API. I had to come up with something that is unique because JmxMonitoredMap uses core hash code for removing mbeans when core is closed. This is unlike the metrics' lifecycle, which survive across core reloads.

          Show
          ab Andrzej Bialecki added a comment - - edited It turned out that it's not a major change - I attached a screenshot of what it looks like in JConsole. Edit: the "_jmx_NNNNNN" is a reporter name in the new API. I had to come up with something that is unique because JmxMonitoredMap uses core hash code for removing mbeans when core is closed. This is unlike the metrics' lifecycle, which survive across core reloads.
          Hide
          ab Andrzej Bialecki added a comment -

          I attached a patch with this modification.

          Show
          ab Andrzej Bialecki added a comment - I attached a patch with this modification.
          Hide
          shalinmangar Shalin Shekhar Mangar added a comment -

          +1 to commit

          Show
          shalinmangar Shalin Shekhar Mangar added a comment - +1 to commit
          Hide
          dsmiley David Smiley added a comment -

          I don't think we should rush the naming on the eve of a feature freeze.

          Thanks for the screenshot; though it's not clear how the patch retains both the old and new hierarchy; can you clarify?

          I don't like "QUERY" nor "QUERYHANDLER" before it. I suggest "SEARCHHANDLER", leaving "Query" terminology across Solr for literally either a Query object or query string to be parsed, but not as a synonym for searching generally. Then SearchHandler could return SEARCHHANDLER... though perhaps at the base level a generic simply HANDLER (I dunno).

          Show
          dsmiley David Smiley added a comment - I don't think we should rush the naming on the eve of a feature freeze. Thanks for the screenshot; though it's not clear how the patch retains both the old and new hierarchy; can you clarify? I don't like "QUERY" nor "QUERYHANDLER" before it. I suggest "SEARCHHANDLER", leaving "Query" terminology across Solr for literally either a Query object or query string to be parsed, but not as a synonym for searching generally. Then SearchHandler could return SEARCHHANDLER... though perhaps at the base level a generic simply HANDLER (I dunno).
          Hide
          ab Andrzej Bialecki added a comment -

          I don't think we should rush the naming on the eve of a feature freeze.

          As I mentioned above, this mainly affects the new API, with a very limited impact on the old API, so I think we should be fine. I'd rather make a couple renames in the next release than to withhold the dozens of renames in this patch that really make the new API much cleaner.

          how the patch retains both the old and new hierarchy

          The same instances of SolrInfoMBean-s are registered under the old names and new hierarchical names. This way we preserve the back-compat, and at the same time we provide an alternative, improved view of the same information.

          though perhaps at the base level a generic simply HANDLER (I dunno).

          Yeah, naming is hard On one hand it needs to be specific enough to be informative, on the other hand it needs to be generic enough to limit the total number of categories, and on the third hand if it's too generic then it becomes meaningless.

          I discussed this with Shalin, we considered and rejected HANDLER, PLUGIN and the like as too generic, after all all these components are handlers and plugins. In the latest patch I moved quite a few components from QUERY to other categories, leaving only those that actually support query-response interaction with users.

          Show
          ab Andrzej Bialecki added a comment - I don't think we should rush the naming on the eve of a feature freeze. As I mentioned above, this mainly affects the new API, with a very limited impact on the old API, so I think we should be fine. I'd rather make a couple renames in the next release than to withhold the dozens of renames in this patch that really make the new API much cleaner. how the patch retains both the old and new hierarchy The same instances of SolrInfoMBean -s are registered under the old names and new hierarchical names. This way we preserve the back-compat, and at the same time we provide an alternative, improved view of the same information. though perhaps at the base level a generic simply HANDLER (I dunno). Yeah, naming is hard On one hand it needs to be specific enough to be informative, on the other hand it needs to be generic enough to limit the total number of categories, and on the third hand if it's too generic then it becomes meaningless. I discussed this with Shalin, we considered and rejected HANDLER, PLUGIN and the like as too generic, after all all these components are handlers and plugins. In the latest patch I moved quite a few components from QUERY to other categories, leaving only those that actually support query-response interaction with users.
          Hide
          ab Andrzej Bialecki added a comment -

          Current patch. Includes the alternative view for SolrInfoMBeans.

          Show
          ab Andrzej Bialecki added a comment - Current patch. Includes the alternative view for SolrInfoMBeans.
          Hide
          dsmiley David Smiley added a comment -

          HANDLER may be a bit too succinct. I suggest REQUESTHANDLER and specifically SEARCHHANDLER (when subclassing SearchHandler) which are actually fairly specific and meaningful. QUERY though.... I dunno what to think of it; one has to guess.

          RE renaming in a subsequent release: ok.

          I see your patch now; ok.

          Show
          dsmiley David Smiley added a comment - HANDLER may be a bit too succinct. I suggest REQUESTHANDLER and specifically SEARCHHANDLER (when subclassing SearchHandler) which are actually fairly specific and meaningful. QUERY though.... I dunno what to think of it; one has to guess. RE renaming in a subsequent release: ok. I see your patch now; ok.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 6c6c077435bcc5bd3f4520a70a4c678d4b3f7661 in lucene-solr's branch refs/heads/master from Andrzej Bialecki
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=6c6c077 ]

          SOLR-9947 Clean up metrics and SolrInfoMBean categories. Add a hierarhical view of
          SolrInfoMBeans in JMX.

          Show
          jira-bot ASF subversion and git services added a comment - Commit 6c6c077435bcc5bd3f4520a70a4c678d4b3f7661 in lucene-solr's branch refs/heads/master from Andrzej Bialecki [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=6c6c077 ] SOLR-9947 Clean up metrics and SolrInfoMBean categories. Add a hierarhical view of SolrInfoMBeans in JMX.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 16452a9f207a8555c3f7870e22dc566c520cbad8 in lucene-solr's branch refs/heads/branch_6x from Andrzej Bialecki
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=16452a9 ]

          SOLR-9947 Clean up metrics and SolrInfoMBean categories. Add a hierarhical view of
          SolrInfoMBeans in JMX.

          (cherry-picked from 6c6c077435bcc5bd3f4520a70a4c678d4b3f7661)

          Show
          jira-bot ASF subversion and git services added a comment - Commit 16452a9f207a8555c3f7870e22dc566c520cbad8 in lucene-solr's branch refs/heads/branch_6x from Andrzej Bialecki [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=16452a9 ] SOLR-9947 Clean up metrics and SolrInfoMBean categories. Add a hierarhical view of SolrInfoMBeans in JMX. (cherry-picked from 6c6c077435bcc5bd3f4520a70a4c678d4b3f7661)
          Hide
          dsmiley David Smiley added a comment -

          Ok I guess my opinion that SEARCHHANDLER is more specific than QUERY isn't shared by you.

          On master branch, we should not register the old names as well... we will likely forget about this in the coming days/weeks and then one day 7.0 will arrive with this cruft still present making it harder to justify removing later. It could be a separate issue marked as blocker for 7.0, or just do in this issue.

          Show
          dsmiley David Smiley added a comment - Ok I guess my opinion that SEARCHHANDLER is more specific than QUERY isn't shared by you. On master branch, we should not register the old names as well... we will likely forget about this in the coming days/weeks and then one day 7.0 will arrive with this cruft still present making it harder to justify removing later. It could be a separate issue marked as blocker for 7.0, or just do in this issue.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit dee7709dd86c64529ee0455d05805ab41b34c736 in lucene-solr's branch refs/heads/master from Shalin Shekhar Mangar
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=dee7709 ]

          SOLR-9947: Fix CloudSolrClientTest.testNonRetryableRequests failure

          Show
          jira-bot ASF subversion and git services added a comment - Commit dee7709dd86c64529ee0455d05805ab41b34c736 in lucene-solr's branch refs/heads/master from Shalin Shekhar Mangar [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=dee7709 ] SOLR-9947 : Fix CloudSolrClientTest.testNonRetryableRequests failure
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit bd80359debc343c1f4969472c6cf30b69639f045 in lucene-solr's branch refs/heads/branch_6x from Shalin Shekhar Mangar
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=bd80359 ]

          SOLR-9947: Fix CloudSolrClientTest.testNonRetryableRequests failure

          (cherry picked from commit dee7709)

          Show
          jira-bot ASF subversion and git services added a comment - Commit bd80359debc343c1f4969472c6cf30b69639f045 in lucene-solr's branch refs/heads/branch_6x from Shalin Shekhar Mangar [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=bd80359 ] SOLR-9947 : Fix CloudSolrClientTest.testNonRetryableRequests failure (cherry picked from commit dee7709)
          Hide
          ab Andrzej Bialecki added a comment -

          Ok I guess my opinion that SEARCHHANDLER is more specific than QUERY isn't shared by you.

          David, if you have strong feelings about this, let's create a separate issue and discuss this. I wanted to complete this round of cleanup before 6.4 is out to avoid leaving some obvious mistakes and omissions that resulted from incremental work across several Jira issues.

          As I explained above, I tried to minimize the number of categories, and at the same time I wanted to reflect the functionality of components... SEARCHHANDLER reflects IMHO an implementation detail, whereas QUERY (I admit, it may sound too generic at first) tries to encompass an idea of components that users use for query / response interaction.

          On master branch, we should not register the old names as well...

          I'll create a separate blocker issue for 7.0. There's a lot of cleanup that still needs to be done in the naming of eg. QPlugin-s, Highlighter-s etc, but that would break back-compat in 6.x, so 7.0 seems like the right place to do it.

          Show
          ab Andrzej Bialecki added a comment - Ok I guess my opinion that SEARCHHANDLER is more specific than QUERY isn't shared by you. David, if you have strong feelings about this, let's create a separate issue and discuss this. I wanted to complete this round of cleanup before 6.4 is out to avoid leaving some obvious mistakes and omissions that resulted from incremental work across several Jira issues. As I explained above, I tried to minimize the number of categories, and at the same time I wanted to reflect the functionality of components... SEARCHHANDLER reflects IMHO an implementation detail, whereas QUERY (I admit, it may sound too generic at first) tries to encompass an idea of components that users use for query / response interaction. On master branch, we should not register the old names as well... I'll create a separate blocker issue for 7.0. There's a lot of cleanup that still needs to be done in the naming of eg. QPlugin-s, Highlighter-s etc, but that would break back-compat in 6.x, so 7.0 seems like the right place to do it.
          Hide
          janhoy Jan Høydahl added a comment -

          The renamings in 6.4 already seem to have caused back-compat problems, see SOLR-10035 and Shawn's comment about external tools that will break.
          Should we not treat the mbeans API in the same way as other APIs and not change before 7.x? We still have time to revert in 6.4.1 (or register same metrics double up under old and new paths?)

          Show
          janhoy Jan Høydahl added a comment - The renamings in 6.4 already seem to have caused back-compat problems, see SOLR-10035 and Shawn's comment about external tools that will break. Should we not treat the mbeans API in the same way as other APIs and not change before 7.x? We still have time to revert in 6.4.1 (or register same metrics double up under old and new paths?)

            People

            • Assignee:
              ab Andrzej Bialecki
              Reporter:
              shalinmangar Shalin Shekhar Mangar
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development