ashcrow / flask-track-usage

Basic metrics tracking for the Flask framework.
Other
53 stars 33 forks source link

Updates for freegeoip function #45

Closed JohnAD closed 6 years ago

JohnAD commented 6 years ago

Because of text limits in SQL fields, the legacy setup cannot work currently; so I was less concerned about backwards compatibility. And there was a global reference to a variable that did not exist. So, I also suspect nobody is using it at the moment.

Anyway, here is my first pass.

In the issue, you had mentioned a legacy parameter. And in one place ipstack mentioned something like that. But the API docs are completely devoid of the that information. Do you know what used to be stored in the ip_info field? Right now, I have it storing a JSON string. But I can't be sure of what used to be stored there (and storing the dict received from the current output from ipstack causes a SQL error in PosgreSQL.)

I'd like to add a test to the test suite. I'll probably have it go to http://www.hostip.info to avoid the need for an API key.

ashcrow commented 6 years ago

In the issue, you had mentioned a legacy parameter. And in one place ipstack mentioned something like that. But the API docs are completely devoid of the that information.

:man_facepalming: docs are hard :laughing:

Do you know what used to be stored in the ip_info field? Right now, I have it storing a JSON string. But I can't be sure of what used to be stored there (and storing the dict received from the current output from ipstack causes a SQL error in PosgreSQL.)

ip_info was the entire output json from freegeoip. It's up to each storage backend to store it as they see fit. It's possible the sql storage wasn't translating it to a string.

I'd like to add a test to the test suite.

:+1:

I'll probably have it go to http://www.hostip.info to avoid the need for an API key.

Looks decent. The only negative is it isn't as verbose ... but it might be good enough for what people want.

JohnAD commented 6 years ago

This should do it. However, I recommend adding another change for v2.0 since it is already breaking: the 128 char field for ip_info is not large enough in SQL storage. Even a paired down data set on an empty loopback barely fits once put into json. Can it be pushed up to, say, 1024 characters?

JohnAD commented 6 years ago

Any chance to review these changes? This could, as you said, be applied to master 1.x. Or, they could be applied to 2.0.

JohnAD commented 6 years ago

changes are made!

The SQL field now "adds keys" until the "json.dumps" is full; that way the field holds as much as possible but is also always valid json. It is hard coded to 128 characters; so we will want to change that line for v2.0.

ashcrow commented 6 years ago

Merging into master so we can cut one more 1.x release with this enhancement, then I'll rebase it into 2.x branch before merging and tagging it.