-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Maps] Display vector tile API response in Console #128922
Conversation
...plugins/console/public/application/containers/editor/legacy/console_editor/editor_output.tsx
Outdated
Show resolved
Hide resolved
Pinging @elastic/kibana-gis (Team:Geo) |
This is insanely cool. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a few maintainability suggestions. Didn't test locally.
...plugins/console/public/application/containers/editor/legacy/console_editor/editor_output.tsx
Outdated
Show resolved
Hide resolved
...plugins/console/public/application/containers/editor/legacy/console_editor/editor_output.tsx
Outdated
Show resolved
Hide resolved
...plugins/console/public/application/containers/editor/legacy/console_editor/editor_output.tsx
Outdated
Show resolved
Hide resolved
...plugins/console/public/application/containers/editor/legacy/console_editor/editor_output.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
core changes LGTM. The fetch function handling the body as array bugger when the content type is a mapbox
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is fantastic! so great to get rid of jQuery, too.
I added a couple of nits, but otherwise lgtm!
code review and tested in chrome
...plugins/console/public/application/containers/editor/legacy/console_editor/editor_output.tsx
Outdated
Show resolved
Hide resolved
...lugins/console/public/application/hooks/use_send_current_request_to_es/send_request_to_es.ts
Outdated
Show resolved
Hide resolved
…y/console_editor/editor_output.tsx Co-authored-by: Nick Peihl <nickpeihl@gmail.com>
…request_to_es/send_request_to_es.ts Co-authored-by: Nick Peihl <nickpeihl@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add a functional test to the https://github.com/elastic/kibana/tree/main/test/functional/apps/console folder to verify _mvt
responses are displayed as expected
Pinging @elastic/platform-deployment-management (Team:Deployment Management) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, being able to view vector tile results in console is such a big help for debugging. Thanks
code review, tested in chrome
it('response should have [meta, aggs, hits] layers', () => { | ||
const vectorTileJson = JSON.parse(convertMapboxVectorTileToJson(vectorTile)); | ||
|
||
expect(vectorTileJson).toHaveProperty('meta'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about using toEqual
to compare against a full object? This will give maintainers more insight into the expected output and will catch silly errors like defining one of these properties with an incorrect value.
@nreese I think this will also address 80% of your request to add a functional test. Functional tests are slower than unit tests and the Console ones have been especially flaky, so maybe we don't need to add functional test coverage if we enhance these units tests. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really sure it's a good idea to compare to a full object, just because the typical response of vector tile data is a pretty big json object.
Here's an example of it, and this is just for one specific coordinate like 8/47/98
:
Click to expand
{
"meta":[
{
"geometry":{
"type":"Polygon",
"coordinates":[
[
[
0,
4096
],
[
4096,
4096
],
[
4096,
0
],
[
0,
0
],
[
0,
4096
]
]
]
},
"properties":{
"_shards.failed":0,
"_shards.skipped":0,
"_shards.successful":1,
"_shards.total":1,
"aggregations._count.avg":5,
"aggregations._count.count":3,
"aggregations._count.max":8,
"aggregations._count.min":3,
"aggregations._count.sum":15,
"hits.total.relation":"eq",
"hits.total.value":15,
"timed_out":false,
"took":67
}
}
],
"hits":[
{
"geometry":{
"type":"Point",
"coordinates":[
3585,
2204
]
},
"properties":{
"_id":"-BbN9X8BEs3pqeJ47-oU",
"_index":"kibana_sample_data_logs"
}
},
{
"geometry":{
"type":"Point",
"coordinates":[
3175,
3576
]
},
"properties":{
"_id":"QhbN9X8BEs3pqeJ47-sU",
"_index":"kibana_sample_data_logs"
}
},
{
"geometry":{
"type":"Point",
"coordinates":[
3585,
2204
]
},
"properties":{
"_id":"GhfN9X8BEs3pqeJ49QJE",
"_index":"kibana_sample_data_logs"
}
},
{
"geometry":{
"type":"Point",
"coordinates":[
3175,
3576
]
},
"properties":{
"_id":"uhfN9X8BEs3pqeJ49w44",
"_index":"kibana_sample_data_logs"
}
},
{
"geometry":{
"type":"Point",
"coordinates":[
3585,
2204
]
},
"properties":{
"_id":"bhfN9X8BEs3pqeJ49xP6",
"_index":"kibana_sample_data_logs"
}
},
{
"geometry":{
"type":"Point",
"coordinates":[
3585,
2204
]
},
"properties":{
"_id":"XhfN9X8BEs3pqeJ4-iNJ",
"_index":"kibana_sample_data_logs"
}
},
{
"geometry":{
"type":"Point",
"coordinates":[
2603,
1476
]
},
"properties":{
"_id":"LxfN9X8BEs3pqeJ4-ykP",
"_index":"kibana_sample_data_logs"
}
},
{
"geometry":{
"type":"Point",
"coordinates":[
2603,
1476
]
},
"properties":{
"_id":"mxfN9X8BEs3pqeJ4-zDa",
"_index":"kibana_sample_data_logs"
}
},
{
"geometry":{
"type":"Point",
"coordinates":[
3585,
2204
]
},
"properties":{
"_id":"dhfN9X8BEs3pqeJ4_zlP",
"_index":"kibana_sample_data_logs"
}
},
{
"geometry":{
"type":"Point",
"coordinates":[
3585,
2204
]
},
"properties":{
"_id":"DRfN9X8BEs3pqeJ4_zpQ",
"_index":"kibana_sample_data_logs"
}
},
{
"geometry":{
"type":"Point",
"coordinates":[
3175,
3576
]
},
"properties":{
"_id":"ZxfO9X8BEs3pqeJ4Akf1",
"_index":"kibana_sample_data_logs"
}
},
{
"geometry":{
"type":"Point",
"coordinates":[
3585,
2204
]
},
"properties":{
"_id":"-xfO9X8BEs3pqeJ4Akf1",
"_index":"kibana_sample_data_logs"
}
},
{
"geometry":{
"type":"Point",
"coordinates":[
3585,
2204
]
},
"properties":{
"_id":"fxfO9X8BEs3pqeJ4ClfI",
"_index":"kibana_sample_data_logs"
}
},
{
"geometry":{
"type":"Point",
"coordinates":[
2603,
1476
]
},
"properties":{
"_id":"fxfO9X8BEs3pqeJ4C1rB",
"_index":"kibana_sample_data_logs"
}
},
{
"geometry":{
"type":"Point",
"coordinates":[
2603,
1476
]
},
"properties":{
"_id":"thfO9X8BEs3pqeJ4DFvc",
"_index":"kibana_sample_data_logs"
}
}
],
"aggs":[
{
"geometry":{
"type":"Polygon",
"coordinates":[
[
[
3584,
2208
],
[
3600,
2208
],
[
3600,
2192
],
[
3584,
2192
],
[
3584,
2208
]
]
]
},
"properties":{
"_key":"16/12256/25225",
"_count":8
}
},
{
"geometry":{
"type":"Polygon",
"coordinates":[
[
[
2592,
1488
],
[
2608,
1488
],
[
2608,
1472
],
[
2592,
1472
],
[
2592,
1488
]
]
]
},
"properties":{
"_key":"16/12194/25180",
"_count":4
}
},
{
"geometry":{
"type":"Polygon",
"coordinates":[
[
[
3168,
3584
],
[
3184,
3584
],
[
3184,
3568
],
[
3168,
3568
],
[
3168,
3584
]
]
]
},
"properties":{
"_key":"16/12230/25311",
"_count":3
}
}
]
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will also address 80% of your request to add a functional test. Functional tests are slower than unit tests and the Console ones have been especially flaky, so maybe we don't need to add functional test coverage if we enhance these units tests.
I am not as concerned with the output format as ensuring vector tile responses continue to work. There are lots of things that could break this that are not tied to the formatting of a vector tile to an output such as blocking/removing http header. The use case for running _mvt
API commands in console is narrow so I think having a functional test to ensure everything continues to work adds a lot of value and is worth test maintenance costs.
I'm not really sure it's a good idea to compare to a full object, just because the typical response of vector tile data is a pretty big json object.
How about adding a query to the sample request so the tile only contains a single hit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are good points, Nathan. I'm +1 on giving a functional test a shot, but I suggest we run it through the flaky test runner. If it turns out to be flaky we can fall back to the unit test.
...plugins/console/public/application/containers/editor/legacy/console_editor/editor_output.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for adding support for vector tile in Console, @maksimkovalev!
Code changes LGTM expect 1 comment about editorOutput
. I tested locally and it seems like that causes an empty response for APIs that return text, for example GET _cat/indices
.
I opened an issue to add more test coverage because this potential regression should have ideally been caught by a failing test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for adding a fix, @maksimkovalev!
Latest changes LGTM, re-tested locally 👍
const PATH_TO_PBF = resolve(__dirname, './response.pbf'); // Query sample "GET kibana_sample_data_logs/_mvt/geo.coordinates/8/47/98" | ||
const PATH_TO_PBF = resolve(__dirname, './response.pbf'); // Query sample "GET kibana_sample_data_logs/_mvt/geo.coordinates/8/23/63" | ||
|
||
const MOCK_JSON = JSON.parse(` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making this change!
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
Ran flaky test runner, https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/447, and there were no failures with new functional tests. |
Closes: #128534
Running the command
get kibana_sample_data_logs/_mvt/geo.coordinates/0/0/0
in Console returns the vector tile source.