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

make timings and metadata headers optional #232

Merged
merged 5 commits into from
Feb 17, 2021
Merged

Conversation

vincentsarago
Copy link
Member

closes #229

Copy link
Member

@kylebarron kylebarron left a comment

Choose a reason for hiding this comment

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

I was just thinking the other day that these should be optional! For example the MDN article says

The Server-Timing header may expose potentially sensitive application and infrastructure information. Consider to control which metrics are returned when and to whom on the server side. For example, you could only show metrics to authenticated users and nothing to the public.

titiler/endpoints/factory.py Show resolved Hide resolved
@vincentsarago
Copy link
Member Author

@kylebarron I've added add_assets_in_headers option in the MosaicTilerFactory. Do it make senses to rename debug -> add_timings_in_headers then ?

@kylebarron
Copy link
Member

You could just have an optional_headers argument that takes a list. Then it'll be more extensible for the future

@vincentsarago
Copy link
Member Author

@kylebarron I'm not sure to fully understand what you mean.

are you saying something like 👇

cog = TilerFactory(optional_headers={"timings", ...}

@kylebarron
Copy link
Member

Yeah basically. You could have an enum of available optional headers. It might also make sense to make the keys the same as the header names, e.g.

cog = TilerFactory(optional_headers={"Server-Timing", "X-Assets", ...}

(though should be case insensitive)

@vincentsarago

This comment has been minimized.

@vincentsarago

This comment has been minimized.

@vincentsarago
Copy link
Member Author

Alright @kylebarron,

You could have an enum of available optional headers.

I don't see how this will be easy to do. basically right now we have "Server-Timing" for basic tilers and "Server-Timing", "X-Assets" for Mosaic tilers. First I'm not sure how to implement this using enums and second we will need two different enums (one only with "Server-Timing" and another with "Server-Timing" and "X-Assets").

I wanted to use attrs so we could have used the attribute validation but attrs makes things a bit more complex (#234)

I don't really want to spend more time on it but feel free to propose your own PR 🙏

Note:
for now I've went the way of 2 different attributes:

  • add_timing_headers
  • add_assets_headers

@kylebarron
Copy link
Member

Not every factory needs to implement all the available optional headers... You can have a single enum listing the optional headers available, and just note in the docs (or maybe give a warning at runtime) if a non-mosaic tiler attempts to use the X-Assets header.

@vincentsarago
Copy link
Member Author

You can have a single enum listing the optional headers available, and just note in the docs (or maybe give a warning at runtime) if a non-mosaic tiler attempts to use the X-Assets header.

seems like more code to me! but also no idea how enum will work for this case when we have multiple values provided as input

@kylebarron
Copy link
Member

Well the enum part isn't important... All I was suggesting is something like this, instead of making a separate parameter for every optional header like add_timing_headers, add_assets_headers, ...

tilerfactory(
	...,
	optional_headers=['Server-Timing', 'X-Assets'])
from . import OptionalHeaders
tilerfactory(
	...,
	optional_headers=[OptionalHeaders.Server_Timing, OptionalHeaders.X_Assets])

@vincentsarago

This comment has been minimized.

@vincentsarago

This comment has been minimized.

@vincentsarago
Copy link
Member Author

Alright it took me some time but I think I understand what you proposed! can you check the last commit and 👍 👎 @kylebarron

@vincentsarago vincentsarago merged commit d93b14a into master Feb 17, 2021
@vincentsarago vincentsarago deleted the optionalTimings branch February 17, 2021 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tiler timings should be optional!
2 participants