Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.5, 6.0
    • Component/s: Facet Module
    • Labels:
      None

      Description

      This is the first step for SOLR-8228 Facet Telemetry. It's going to implement the telemetry for a nested facet query and put the information obtained in debug field in response.

      Here is an example of telemetry returned from query.
      Query

      curl http://localhost:8228/solr/films/select -d 'q=*:*&wt=json&indent=true&debugQuery=true&json.facet={
      top_genre: {
        type:terms,
        field:genre,
        numBucket:true,
        limit:2,
        facet: {
          top_director: {
              type:terms,
              field:directed_by,
              numBuckets:true,
              limit:2
          },
          first_release: {
              type:terms,
              field:initial_release_date,
              sort:{index:asc},
              numBuckets:true,
              limit:2
          }
        }
      }
      }'
      

      Telemetry returned (inside debug part)

          "facet-trace":{
            "processor":"FacetQueryProcessor",
            "elapse":1,
            "query":null,
            "sub-facet":[{
                "processor":"FacetFieldProcessorUIF",
                "elapse":1,
                "field":"genre",
                "limit":2,
                "sub-facet":[{
                    "filter":"genre:Drama",
                    "processor":"FacetFieldProcessorUIF",
                    "elapse":0,
                    "field":"directed_by",
                    "limit":2},
                  {
                    "filter":"genre:Drama",
                    "processor":"FacetFieldProcessorNumeric",
                    "elapse":0,
                    "field":"initial_release_date",
                    "limit":2},
                  {
                    "filter":"genre:Comedy",
                    "processor":"FacetFieldProcessorUIF",
                    "elapse":0,
                    "field":"directed_by",
                    "limit":2},
                  {
                    "filter":"genre:Comedy",
                    "processor":"FacetFieldProcessorNumeric",
                    "elapse":0,
                    "field":"initial_release_date",
                    "limit":2}]}]},
      
      1. SOLR-8230.patch
        13 kB
        Michael Sun
      2. SOLR-8230.patch
        14 kB
        Michael Sun
      3. SOLR-8230.patch
        13 kB
        Michael Sun
      4. SOLR-8230.patch
        13 kB
        Michael Sun
      5. SOLR-8230.patch
        13 kB
        Michael Sun

        Activity

        Hide
        Michael Sun added a comment -

        Uploaded first patch for review. It adds a facet trace field in response to reveal the debugging information of facet if debugQuery=true in request. Here is some thoughts for the patch.

        1. In the patch, it shows the facet processor used, elapse time and facet description for each step to execute facet. If there is sub-facet, it shows the facet hierarchy as well. It helps to understand how the facet is executed and the cost of each step.
        2. The total count and unique count is not included and is planned for next sub-task.
        3. The debug information is stored along with FacetContext since FacetContext maintains steps and hierarchy of facet execution. FacetContext is organized in a tree structure. The root of the tree is stored in ResponseBuilder as "FacetContext".
        4. FacetDebugInfo.getFacetDebugInfoInJSON() is designed to be static. The reason is FacetContext is package private. From DebugComponent.java, there is no good way to access facet debug information.
        5. The escape string in JSON output format in facet trace in the response is somehow not completely correct. I am trying to figure it out.

        Show
        Michael Sun added a comment - Uploaded first patch for review. It adds a facet trace field in response to reveal the debugging information of facet if debugQuery=true in request. Here is some thoughts for the patch. 1. In the patch, it shows the facet processor used, elapse time and facet description for each step to execute facet. If there is sub-facet, it shows the facet hierarchy as well. It helps to understand how the facet is executed and the cost of each step. 2. The total count and unique count is not included and is planned for next sub-task. 3. The debug information is stored along with FacetContext since FacetContext maintains steps and hierarchy of facet execution. FacetContext is organized in a tree structure. The root of the tree is stored in ResponseBuilder as "FacetContext". 4. FacetDebugInfo.getFacetDebugInfoInJSON() is designed to be static. The reason is FacetContext is package private. From DebugComponent.java, there is no good way to access facet debug information. 5. The escape string in JSON output format in facet trace in the response is somehow not completely correct. I am trying to figure it out.
        Hide
        Yonik Seeley added a comment -

        OK, so I had a quick look...

        5. The escape string in JSON output format in facet trace in the response is somehow not completely correct. I am trying to figure it out.

        Don't serialize it yourself... You should add a general object to the response (Map, List, etc) and let the response writer formulate the correct response (JSON, binary, XML, etc).

        Show
        Yonik Seeley added a comment - OK, so I had a quick look... 5. The escape string in JSON output format in facet trace in the response is somehow not completely correct. I am trying to figure it out. Don't serialize it yourself... You should add a general object to the response (Map, List, etc) and let the response writer formulate the correct response (JSON, binary, XML, etc).
        Hide
        Michael Sun added a comment -

        Make sense. Thanks Yonik Seeley

        Show
        Michael Sun added a comment - Make sense. Thanks Yonik Seeley
        Hide
        Michael Sun added a comment -

        Just uploaded a new patch. Here is the change after previous patch.
        1. the facet debug information is organized as general objects (map and list) and is formulated by response writer. In this way, the output trace is in correct format.
        2. add filter description for each sub-facet processed.

        Show
        Michael Sun added a comment - Just uploaded a new patch. Here is the change after previous patch. 1. the facet debug information is organized as general objects (map and list) and is formulated by response writer. In this way, the output trace is in correct format. 2. add filter description for each sub-facet processed.
        Hide
        Yonik Seeley added a comment -

        I like the getFacetDescription() in FacetRequest, it's nice and general. Perhaps we should make a generic base-class implementation of FacetRequest.toString() that uses this as well.

        In FacetContext, I see you build a tree - having the current context point to all it's sub-contexts (even when debug isn't turned on).
        That may be problematic memory-wise... FacetContext contains a reference to (among other things) "base", the DocSet for the facet domain. Previously FacetContext objects were not retained longer than needed.

        Show
        Yonik Seeley added a comment - I like the getFacetDescription() in FacetRequest, it's nice and general. Perhaps we should make a generic base-class implementation of FacetRequest.toString() that uses this as well. In FacetContext, I see you build a tree - having the current context point to all it's sub-contexts (even when debug isn't turned on). That may be problematic memory-wise... FacetContext contains a reference to (among other things) "base", the DocSet for the facet domain. Previously FacetContext objects were not retained longer than needed.
        Hide
        Michael Sun added a comment -

        Thanks Yonik Seeley. It's really good insight to point out the potential memory issue. Let me fix it. It can be fixed either by building a separate tree for telemetry information only (small amount of data) or null out the unused fields in sub context once it's used.

        Show
        Michael Sun added a comment - Thanks Yonik Seeley . It's really good insight to point out the potential memory issue. Let me fix it. It can be fixed either by building a separate tree for telemetry information only (small amount of data) or null out the unused fields in sub context once it's used.
        Hide
        Michael Sun added a comment -

        Just uploaded new patch. In the patch, it builds a tree structure for facet telemetry information only during facet processing. The facet context is not retained after each facet is processed as it was.

        Show
        Michael Sun added a comment - Just uploaded new patch. In the patch, it builds a tree structure for facet telemetry information only during facet processing. The facet context is not retained after each facet is processed as it was.
        Hide
        Michael Sun added a comment -

        Just uploaded a new patch. Thanks Yonik Seeley for review. Here is the change from last patch.

        1. If debug option in query is not set, FacetDebugInfo is not created to reduce memory usage.
        2. It allows to add user defined information into FacetDebugInfo.

        Also an example facet telemetry is added in JIRA.

        Show
        Michael Sun added a comment - Just uploaded a new patch. Thanks Yonik Seeley for review. Here is the change from last patch. 1. If debug option in query is not set, FacetDebugInfo is not created to reduce memory usage. 2. It allows to add user defined information into FacetDebugInfo. Also an example facet telemetry is added in JIRA.
        Hide
        Yonik Seeley added a comment -

        Is this patch for trunk? I'm getting failures when I try to apply it.

        Show
        Yonik Seeley added a comment - Is this patch for trunk? I'm getting failures when I try to apply it.
        Hide
        Michael Sun added a comment -

        Yonik Seeley The patch is on trunk.

        Show
        Michael Sun added a comment - Yonik Seeley The patch is on trunk.
        Hide
        Michael Sun added a comment -

        Attach an updated patch generated from trunk up to date.

        Show
        Michael Sun added a comment - Attach an updated patch generated from trunk up to date.
        Hide
        ASF subversion and git services added a comment -

        Commit 1720823 from Yonik Seeley in branch 'dev/trunk'
        [ https://svn.apache.org/r1720823 ]

        SOLR-8230: JSON Facet API: add facet-info to debug when debugQuery=true

        Show
        ASF subversion and git services added a comment - Commit 1720823 from Yonik Seeley in branch 'dev/trunk' [ https://svn.apache.org/r1720823 ] SOLR-8230 : JSON Facet API: add facet-info to debug when debugQuery=true
        Hide
        ASF subversion and git services added a comment -

        Commit 1720824 from Yonik Seeley in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1720824 ]

        SOLR-8230: JSON Facet API: add facet-info to debug when debugQuery=true

        Show
        ASF subversion and git services added a comment - Commit 1720824 from Yonik Seeley in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1720824 ] SOLR-8230 : JSON Facet API: add facet-info to debug when debugQuery=true
        Hide
        Yonik Seeley added a comment -

        Committed. Thanks Michael!
        I also added a simple test to just test for the presence of "facet-info" and also randomly added it in the main TestJsonFacets test just to ensure that it didn't cause exceptions or other issues for all the various facet types.

        From a style perspective, I also moved the license to the top of the new file.

        Show
        Yonik Seeley added a comment - Committed. Thanks Michael! I also added a simple test to just test for the presence of "facet-info" and also randomly added it in the main TestJsonFacets test just to ensure that it didn't cause exceptions or other issues for all the various facet types. From a style perspective, I also moved the license to the top of the new file.

          People

          • Assignee:
            Unassigned
            Reporter:
            Michael Sun
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development