Solr
  1. Solr
  2. SOLR-1665

Add &debugTimings param so that timings for components can be retrieved without having to do explains(), as in &debugQuery

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.0-ALPHA
    • Component/s: None
    • Labels:
      None

      Description

      As the title says, it would be great if we could just get back component timings w/o having to do the full boat of explains and other stuff.

      1. SOLR-1665.patch
        8 kB
        Grant Ingersoll
      2. SOLR-1665.patch
        6 kB
        Grant Ingersoll
      3. SOLR-1665.patch
        45 kB
        Grant Ingersoll
      4. SOLR-1665.patch
        42 kB
        Grant Ingersoll

        Activity

        Hide
        Erik Hatcher added a comment -

        Just an idea, how about refactoring how the debug parameter works altogether, so instead of just adding more debugFoo parameters, we can have debug=explain,query,timing sort of thing?

        Anyway, it's a syntactic debate, not sure which I prefer (though I do kinda dislike camelCased query string parameters).

        Show
        Erik Hatcher added a comment - Just an idea, how about refactoring how the debug parameter works altogether, so instead of just adding more debugFoo parameters, we can have debug=explain,query,timing sort of thing? Anyway, it's a syntactic debate, not sure which I prefer (though I do kinda dislike camelCased query string parameters).
        Hide
        Yonik Seeley added a comment -

        bq? As the title says, it would be great if we could just get back component timings w/o having to do the full boat of explains and other stuff.

        If it's just for debugging, why does it matter? Or are you considering running in production?

        Just an idea, how about refactoring how the debug parameter works altogether, so instead of just adding more debugFoo parameters, we can have debug=explain,query,timing sort of thing?

        That might be a good idea - fewer hash lookups on every request for paramters that don't matter most of the time.

        Show
        Yonik Seeley added a comment - bq? As the title says, it would be great if we could just get back component timings w/o having to do the full boat of explains and other stuff. If it's just for debugging, why does it matter? Or are you considering running in production? Just an idea, how about refactoring how the debug parameter works altogether, so instead of just adding more debugFoo parameters, we can have debug=explain,query,timing sort of thing? That might be a good idea - fewer hash lookups on every request for paramters that don't matter most of the time.
        Hide
        Grant Ingersoll added a comment -

        +1 to Erik's idea.

        If it's just for debugging, why does it matter? Or are you considering running in production?

        I'd say it's pseudo-production, if that makes any sense. I may want to collect timings of components under some load (either production or in a test environment), or, simply for long running queries where I only care about the timings, I don't want to spend the time on explains. It's not a huge issue, but have thought it would be helpful on occasion.

        Show
        Grant Ingersoll added a comment - +1 to Erik's idea. If it's just for debugging, why does it matter? Or are you considering running in production? I'd say it's pseudo-production, if that makes any sense. I may want to collect timings of components under some load (either production or in a test environment), or, simply for long running queries where I only care about the timings, I don't want to spend the time on explains. It's not a huge issue, but have thought it would be helpful on occasion.
        Hide
        Hoss Man added a comment -

        please, please, please...

        "debug=explain&debug=query&debug=timing" ... if "true" is in the list, then all known types are used. if "false" is in the list, then all other items in the list are ignored.

        ...and yes, this seems like a good idea in general

        Show
        Hoss Man added a comment - please, please, please... "debug=explain&debug=query&debug=timing" ... if "true" is in the list, then all known types are used. if "false" is in the list, then all other items in the list are ignored. ...and yes, this seems like a good idea in general
        Hide
        Jason Rutherglen added a comment -

        Plus one, visibility into the components would be good. This'll work for distributed processes (i.e. time taken on each node per component)?

        Show
        Jason Rutherglen added a comment - Plus one, visibility into the components would be good. This'll work for distributed processes (i.e. time taken on each node per component)?
        Hide
        Hoss Man added a comment -

        Bulk updating 240 Solr issues to set the Fix Version to "next" per the process outlined in this email...

        http://mail-archives.apache.org/mod_mbox/lucene-dev/201005.mbox/%3Calpine.DEB.1.10.1005251052040.24672@radix.cryptio.net%3E

        Selection criteria was "Unresolved" with a Fix Version of 1.5, 1.6, 3.1, or 4.0. email notifications were suppressed.

        A unique token for finding these 240 issues in the future: hossversioncleanup20100527

        Show
        Hoss Man added a comment - Bulk updating 240 Solr issues to set the Fix Version to "next" per the process outlined in this email... http://mail-archives.apache.org/mod_mbox/lucene-dev/201005.mbox/%3Calpine.DEB.1.10.1005251052040.24672@radix.cryptio.net%3E Selection criteria was "Unresolved" with a Fix Version of 1.5, 1.6, 3.1, or 4.0. email notifications were suppressed. A unique token for finding these 240 issues in the future: hossversioncleanup20100527
        Hide
        Grant Ingersoll added a comment -

        Here's a patch for this. Adds support for debug=timing, debug=results (explains for now, but could be other things in the future) and debug=query. Still supports debugQuery=true (back compat) and also now supports debug=true (same as debugQuery).

        Should also be extensible so that people could add in their own custom values as well for their components.

        Show
        Grant Ingersoll added a comment - Here's a patch for this. Adds support for debug=timing, debug=results (explains for now, but could be other things in the future) and debug=query. Still supports debugQuery=true (back compat) and also now supports debug=true (same as debugQuery). Should also be extensible so that people could add in their own custom values as well for their components.
        Hide
        Grant Ingersoll added a comment -

        I intend to commit in a day or two.

        Show
        Grant Ingersoll added a comment - I intend to commit in a day or two.
        Hide
        Yonik Seeley added a comment -

        This seems a bit over-engineered?
        What's the point of having a "Set<String> debugInterests" and doing a lookup on that rather than just doing a lookup on the params?

        And changing addDebugInfo(String interest, String name, Object val )
        so that it checks debugInterest to see if the info should be added? One of the main points is to not generate debug info if it's not asked for.

        I'd almost prefer taking a step back and implementing only what the original issue talked about: add a debugTimings param that did timings only - that's the special case and the only one we've identified that makes sense.

        Show
        Yonik Seeley added a comment - This seems a bit over-engineered? What's the point of having a "Set<String> debugInterests" and doing a lookup on that rather than just doing a lookup on the params? And changing addDebugInfo(String interest, String name, Object val ) so that it checks debugInterest to see if the info should be added? One of the main points is to not generate debug info if it's not asked for. I'd almost prefer taking a step back and implementing only what the original issue talked about: add a debugTimings param that did timings only - that's the special case and the only one we've identified that makes sense.
        Hide
        Grant Ingersoll added a comment -

        I was just going to do a enum type, but then figured why not let others extend. FWIW, we did identify three cases: timings, results/explains and queries, and that is what I did.

        And changing addDebugInfo(String interest, String name, Object val )

        so that it checks debugInterest to see if the info should be added? One of the main points is to not generate debug info if it's not asked for.

        ResponseBuilder.addDebugInfo() w/o the interest being passed in has no context as to whether it should be called or not. Moving the if clause to be around the call to addDebugInfo instead of in it then requires the caller to have knowledge of the debug interests. That being said, I suppose if we just do param lookup each time, it's not a big deal.

        Show
        Grant Ingersoll added a comment - I was just going to do a enum type, but then figured why not let others extend. FWIW, we did identify three cases: timings, results/explains and queries, and that is what I did. And changing addDebugInfo(String interest, String name, Object val ) so that it checks debugInterest to see if the info should be added? One of the main points is to not generate debug info if it's not asked for. ResponseBuilder.addDebugInfo() w/o the interest being passed in has no context as to whether it should be called or not. Moving the if clause to be around the call to addDebugInfo instead of in it then requires the caller to have knowledge of the debug interests. That being said, I suppose if we just do param lookup each time, it's not a big deal.
        Hide
        Yonik Seeley added a comment -

        ResponseBuilder.addDebugInfo() w/o the interest being passed in has no context as to whether it should be called or not.

        I'm not sure I understand. It seems to me like addDebugInfo() should just add the debug info? It's up to the caller not to call it if debugging isn't enabled.

        So how about just adding an additional "boolean debugTiming" to the response builder?
        Then places that are timing related can just be switched from checking debug to checking debugTiming?

        Show
        Yonik Seeley added a comment - ResponseBuilder.addDebugInfo() w/o the interest being passed in has no context as to whether it should be called or not. I'm not sure I understand. It seems to me like addDebugInfo() should just add the debug info? It's up to the caller not to call it if debugging isn't enabled. So how about just adding an additional "boolean debugTiming" to the response builder? Then places that are timing related can just be switched from checking debug to checking debugTiming?
        Hide
        Grant Ingersoll added a comment -

        Here's a simpler version w/o the expandability

        Show
        Grant Ingersoll added a comment - Here's a simpler version w/o the expandability
        Hide
        Grant Ingersoll added a comment -

        Committed revision 990577.

        Show
        Grant Ingersoll added a comment - Committed revision 990577.
        Hide
        Yonik Seeley added a comment -

        TODO: this doesn't work with distributed search.
        It's not critical or anything since distributed search appears to still work as it always has, but the new debug params are ignored.

        The following request demonstrates:
        http://localhost:8983/solr/select?q=solr&debug=timing&shards=localhost:8983/solr

        I've reopened this issue to track since it's hopefully easy enough that it doesn't warrant it's own issue.

        Show
        Yonik Seeley added a comment - TODO: this doesn't work with distributed search. It's not critical or anything since distributed search appears to still work as it always has, but the new debug params are ignored. The following request demonstrates: http://localhost:8983/solr/select?q=solr&debug=timing&shards=localhost:8983/solr I've reopened this issue to track since it's hopefully easy enough that it doesn't warrant it's own issue.
        Hide
        Grant Ingersoll added a comment -

        Argh, I always forget the distributed stuff. I'll take a crack at fixing it.

        Show
        Grant Ingersoll added a comment - Argh, I always forget the distributed stuff. I'll take a crack at fixing it.
        Hide
        Grant Ingersoll added a comment -

        Adds distributed support.

        Show
        Grant Ingersoll added a comment - Adds distributed support.
        Hide
        Grant Ingersoll added a comment -

        patch for distributed, plust test

        Show
        Grant Ingersoll added a comment - patch for distributed, plust test
        Hide
        Grant Ingersoll added a comment -

        I added in distributed support, plus a test.

        Show
        Grant Ingersoll added a comment - I added in distributed support, plus a test.
        Hide
        Yonik Seeley added a comment -

        FYI, this broke the MLT handler.
        I just committed a fix, and will commit a test in conjunction with SOLR-2107

        Show
        Yonik Seeley added a comment - FYI, this broke the MLT handler. I just committed a fix, and will commit a test in conjunction with SOLR-2107
        Hide
        Yonik Seeley added a comment -

        Due to the cost of distributed search tests, I removed DistributedDebugComponentTest and moved the debug tests to TestDistributedSearch.

        Show
        Yonik Seeley added a comment - Due to the cost of distributed search tests, I removed DistributedDebugComponentTest and moved the debug tests to TestDistributedSearch.

          People

          • Assignee:
            Grant Ingersoll
            Reporter:
            Grant Ingersoll
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development