Conversation
b33f34d
to
8ca76e3
Compare
|
||
While Martin, in particular, seemed like a compelling solution, we had enough | ||
questions about using it to discourage us from taking on the complexity of | ||
using it here. |
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 am very curious about Martin (et al), and I think others are too! We discussed that this would be a good R&D opportunity. Can those who are more involved with this project explain why this project in and of itself can't be that R&D opportunity? What are the limitations I don't know about, and can they be further discussed/captured?
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.
@jwalgran may amplify or correct this but I think one main reason not to have it here is that the client needs to be able to request subsets of facilities using query parameters like contributor id or facility name etc.
If we added Martin, making changes to these queries (like adding an ability to search on facility description or certifications) would require us to update both the Django queryset filter code and also the PL/pgSQL code used in Martin.
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.
Yep. If we didn't have to filter, Martin would be a more attractive choice. Writing our own endpoint also allows us to bail out of using ST_AsMVT
if it proves to be a problem and do our tile rendering on an app server rather than in the database.
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, makes sense. Consider adding the above to the ADR
##### Configuration | ||
|
||
Martin appears fairly straightforward to configure and its documentation | ||
encompassed most of what we'd want to do. |
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.
the power of good docs 🌦
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 an excellent summary of our team discussion. Thanks.
858b4ea
to
59cf56a
Compare
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.
Nice work! Very detailed.
@@ -7,6 +7,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. | |||
|
|||
## [Unreleased] | |||
### Added | |||
- Add vector tile ADR [#723](https://github.com/open-apparel-registry/open-apparel-registry/pull/723) |
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.
Consider linking to the ADR itself
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.
Good suggestion -- I'm going to leave this link as is since custom has been to link to the PR.
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 seems like a good approach given the surrounding context, and I'm interested to see how it plays out.
I don't feel strongly that any of my comments need to be integrated prior to merge, but they were an attempt to add additional clarity since the repository is open source and may be consumed by people outside of Azavea.
we also want users to be able to filter these vector tiles by query parameters | ||
like contributor, facility name, and country, along with the map bounding box. | ||
|
||
To accomplish this we have decided to use vector tiles generated, ultimately, |
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 begins to leak details about the decision. I would consider moving this and some of the details below it into the Decision section.
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.
Makes sense -- I'll pare this down a bit here and move things into "Decision" or whatever is the relevant section.
### Reusing Existing /facilities API Endpoint | ||
|
||
In theory we could remove the `MAX_PAGE_SIZE` limit on the `/facilities` API | ||
endpoint. In practice this would cause performance problems as the size of the |
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.
If you are familiar with the pitfalls of large GeoJSON payloads and the browser's ability to render them, you read those details into this sentence as you skim it. To make it clearer, I think it would be helpful to consider citing a specific reason why performance problems are expected for the HTTP request attempting to serve the large GeoJSON payload, and the associated rendering of it on the client.
For example, the large GeoJSON HTTP payload is expected to be in the MBs (speculating), which consumes bandwidth and incurs a fairly significant serialization cost. Similarly, the client-side rendering will be expensive due to the sheer number of Leaflet markers that need to be rendered.
|
||
While we could potentially use a combination of [Windshaft][windshaft] and | ||
[Leaflet.utfgrid][leaflet-utfgrid] to render facilities, there wasn't much | ||
enthusiasm for setting up and maintaining a Windshaft tiler. |
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.
Enthusiasm doesn't seem like the best justification for not going with a particular solution. I would consider focusing on the aspects of the tool lead to a lack of enthusiasm (Terence probably has the best context for this):
- The project is not well-maintained or documented
- It requires an amount of configuration that is excessive for this use case
- It requires adding another service vs. reusing the Django application
- Something about UTF grid
|
||
### Creating Static Vector Tiles | ||
|
||
We ruled out the idea of creating a static set of vector tiles because the |
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.
Good justification.
or search option to the web application, we'd have to write a version of the | ||
same query in PL/pgSQL for the tiler. | ||
|
||
##### Security |
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.
In addition to this, I think integrating another service from a request authorization perspective is a significant con. For example, if the existing API supports API keys for authentication, a tile service would have to replicate that authorization logic, or be reverse proxied through Django to reuse it (opening the door to the same performance bottleneck issues cited as cons for ST_AsMVT
).
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.
Going to add this to the Security
section:
Likewise, adding PostGIS-based security just for the tiler may also compel us to
have to figure out how to duplicate features like API key authentication or
facilities-data request logging -- which we've already written once in Django.
|
||
## Consequences | ||
|
||
As a consequence of this decision, we will need to: |
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.
Potentially too low detail for the ADR, but we may also need to consider:
- Tweaking the number of Gunicorn workers per Gunicorn instance, in addition to the Gunicorn worker type (synchronous vs. Gevent). This would be in service of trying to increase the concurrent request capabilities of a single Gunicorn instance vs. spinning up more of them.
- Looking at caching HTTP headers used by Whitenoise for static assets and evaluating which make sense to replicate for
/tile
. - Extending caching behaviors in CloudFront so that they respect any caching header changes made at the Django level.
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.
Sounds good -- I'll add line items about Gunicorn configuration & caching to the list.
ad308a0
to
e93ca56
Compare
e93ca56
to
7c31b21
Compare
Thanks for the thoughtful reviews! |
application's complexity. Keeping the tile endpoint in Django does not require | ||
adding a new service. | ||
|
||
##### Allows Scaling by Increasing the Number of App Instances |
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 don't think this warrants a change to this ADR nor the decision you've arrived at, but it may be valuable to point out that this may not be a pro (it may not be a con either, but is certainly a consequence). I understood the implication of this statement to be that you lose the ability to scale the the tiling and the API endpoints separately, they must be done in tandem. You also lose the ability to provision the API infrastructure independently of the tiler. For example, if the tiling endpoints end up requiring higher memory allocations, you will be obligated to provision all API instances with the extra resources, and will take that hit if you need to scale on the API-only dimension.
Overview
Adds an ADR documenting our decision to add an
ST_AsMVT
-backed vector tile endpoint to the existing Django web app.Connects #704
Testing Instructions
Checklist
fixup!
commits have been squashed