Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixing map bounds #25040

Merged
merged 1 commit into from
Nov 16, 2018
Merged

fixing map bounds #25040

merged 1 commit into from
Nov 16, 2018

Conversation

ppisljar
Copy link
Member

@ppisljar ppisljar commented Nov 2, 2018

Summary

Tries to resolve #23261

during aggconfig refactoring this part was not updated to use the new way to create AggConfig.
we should no longer create AggConfig's maually but should use aggs.createAggConfig method.

it seems that in this case we don't want to add this aggregation to the list of all aggs thats why we call it with { addToAggConfigs: false }

however after this fix i no longer receive error messages, but it also seems that the fit bounds is now behaving as described in original issue ...

@thomasneirynck could you double check this ? i am on vacation so it might take a bit before i get back to this.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@ppisljar ppisljar added the WIP Work in progress label Nov 2, 2018
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@lukeelmers
Copy link
Member

lukeelmers commented Nov 2, 2018

after testing this PR, i found that it removes the undefined error (yay!) but re-introduces the bug that was described in the original issue (as @ppisljar describes above). i'm diving deeper to see if i can sort this out.

here's a quick comparison of this behavior on the current PR vs master:

this PR...
pr-25040

master...
master

@lukeelmers
Copy link
Member

I'm also noticing the query that courier sends to ES upon clicking to update bounds is missing the geo_bounding_box:

on master, this PR, and 6.4:

  "query": {
    "bool": {
      "must": [
        {
          "match_all": {}
        }
      ],
      "filter": [],
      "should": [],
      "must_not": []
    }
  }

on 6.3:

  "query": {
    "bool": {
      "must": [
        {
          "match_all": {}
        },
        {
          "match_all": {}
        },
        {
          "geo_bounding_box": {
            "ignore_unmapped": true,
            "geo.coordinates": {
              "bottom_right": {
                "lat": 44.33956524809713,
                "lon": -120.58593750000001
              },
              "top_left": {
                "lat": 51.17934297928929,
                "lon": -125.85937500000001
              }
            }
          }
        },
        {
          "range": {
            "@timestamp": {
              "gte": 1541196237041,
              "lte": 1541197137041,
              "format": "epoch_millis"
            }
          }
        }
      ],
      "filter": [],
      "should": [],
      "must_not": []
    }
  },

@ppisljar
Copy link
Member Author

ppisljar commented Nov 5, 2018

and that info is not in filter agg ? full request:

{
  "aggs": {
    "filter_agg": {
      "filter": {
        "geo_bounding_box": {
          "ignore_unmapped": true,
          "geo.coordinates": {
            "top_left": {
              "lat": 66.60225,
              "lon": -166.706545
            },
            "bottom_right": {
              "lat": 12.040769999999998,
              "lon": -53.767084999999994
            }
          }
        }
      },
      "aggs": {
        "2": {
          "geohash_grid": {
            "field": "geo.coordinates",
            "precision": 3
          },
          "aggs": {
            "3": {
              "geo_centroid": {
                "field": "geo.coordinates"
              }
            }
          }
        }
      }
    }
  },
  "size": 0,
  "_source": {
    "excludes": []
  },
  "stored_fields": [
    "*"
  ],
  "script_fields": {},
  "docvalue_fields": [
    {
      "field": "@timestamp",
      "format": "date_time"
    },
    {
      "field": "relatedContent.article:modified_time",
      "format": "date_time"
    },
    {
      "field": "relatedContent.article:published_time",
      "format": "date_time"
    },
    {
      "field": "utc_time",
      "format": "date_time"
    }
  ],
  "query": {
    "bool": {
      "must": [
        {
          "match_all": {}
        },
        {
          "range": {
            "@timestamp": {
              "gte": 1540802241390,
              "lte": 1541407041390,
              "format": "epoch_millis"
            }
          }
        }
      ],
      "filter": [],
      "should": [],
      "must_not": []
    }
  }
}

@thomasneirynck
Copy link
Contributor

@lukeelmers

I think this actually fixes the issue (on master at least). The Cannot read property reduce of undefined error was due to the AggConfig no longer having the .vis property and not using the new API correctly, which this PR resolves.

But this doesn't explain your first gif!!!! So basically, I cannot reproduce that part....

Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After rebasing on the latest master, things are now working for me as expected -- the zooming to fit bounds is functioning correctly, and the toast is still gone. This is consistent with what @thomasneirynck reported yesterday.

@ppisljar If everything is working on your end too I think we are good to go here.

@ppisljar ppisljar added review v7.0.0 v6.6.0 release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure and removed WIP Work in progress labels Nov 12, 2018
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@ppisljar ppisljar merged commit 6f52761 into elastic:master Nov 16, 2018
ppisljar added a commit to ppisljar/kibana that referenced this pull request Nov 16, 2018
ppisljar added a commit to ppisljar/kibana that referenced this pull request Nov 16, 2018
@ppisljar ppisljar deleted the fix/mapsBounds branch November 16, 2018 06:30
ppisljar added a commit that referenced this pull request Nov 16, 2018
ppisljar added a commit that referenced this pull request Nov 16, 2018
@thomasneirynck
Copy link
Contributor

@ppisljar can this be backported to v6.5.1 as well?

@lukeelmers
Copy link
Member

@thomasneirynck I think it was backported in #25787 so it should be in v6.5.1 or v6.5.2 at this point.

@marcosnr
Copy link

FYI, same error and screenshot following "getting started on kibana" video, adding sample data of flights, then adding a point map and trying to zoom... using elasticcloud default cluster template...Version: 6.5.1 latest (cross post from #23261)

@thomasneirynck
Copy link
Contributor

@ppisljar was this backported to 6.5.1?

@ppisljar
Copy link
Member Author

not sure it made it to 6.5.1 but it was merged into 6.5 branch 13 days ago.

@thomasneirynck
Copy link
Contributor

great thx @ppisljar !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:fix review Team:Visualizations Visualization editors, elastic-charts and infrastructure v6.5.1 v6.6.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Fit Data Bounds" neglects filter settings
5 participants