Skip to content
This repository has been archived by the owner on Jul 26, 2024. It is now read-only.

feat: include ip addr for diagnosing mmdb lookup failures #154

Merged
merged 1 commit into from
Jun 14, 2021
Merged

Conversation

pjenvey
Copy link
Member

@pjenvey pjenvey commented Jun 14, 2021

@pjenvey pjenvey requested review from jbuck and a team June 14, 2021 19:46
@@ -44,7 +44,12 @@ pub async fn get_tiles(
let addr = match ip_addr_str.parse() {
Ok(v) => v,
Err(e) => {
return Err(HandlerErrorKind::General(format!("Invalid remote IP {:?}", e)).into());
// Temporary: log the IP addr for debugging mmdb issues
return Err(HandlerErrorKind::General(format!(
Copy link
Contributor

Choose a reason for hiding this comment

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

Since IPs are relatively sensitive information, should this be a configurable option that defaults to off? We could then turn it back off without having to cut a new release? (Or is there some other plan that isn't linked to from here?)

Copy link
Member Author

@pjenvey pjenvey Jun 14, 2021

Choose a reason for hiding this comment

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

I can but I'm not intending for this to be used outside of the dev env (or worse case stage if we don't reproduce it there).

I'm working on a change for #148 that will land shortly where I'll be intending to revert his

Copy link
Contributor

@mythmon mythmon left a comment

Choose a reason for hiding this comment

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

The code changes look good. I trust that we'll be careful to not let this ride to prod.

@mythmon mythmon dismissed their stale review June 14, 2021 20:23

Removing blocker

@pjenvey pjenvey merged commit 4f1d61f into main Jun 14, 2021
@pjenvey pjenvey deleted the feat/149 branch June 14, 2021 20:25
Trinaa pushed a commit that referenced this pull request Jun 13, 2022
Add support for multiple API endpoints 📱
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants