Support ISP database for ASN data and move parts to Java #111

suyograo · 2017-04-20T00:14:36Z

This adds support to GeoIP2 ISP database which includes the ASN data.

Also, while doing this, it made sense to me to just move most parts to Java since we were already using the java APIs and not the ruby libs. All existing tests pass.

suyograo · 2017-04-20T00:18:50Z

lib/logstash/filters/geoip.rb

-                                                    'dma_code', 'ip', 'latitude',
-                                                    'longitude', 'postal_code', 'region_name',
-                                                    'region_code', 'timezone', 'location']
+  config :fields, :validate => :array


The docs still mention the same defaults, but the actual defaults have moved to Java enum class.

Not listing a :default here makes the documentation generator (which will change soon with @ph's work) show that the setting has no default value. Could be confusing.

However, I think given @ph/@dede's efforts to change how plugins get documented, this is probably a fine change.

(Mostly noting this for posterity)

suyograo · 2017-04-20T00:19:05Z

lib/logstash/filters/geoip.rb

@@ -113,132 +96,44 @@ class LogStash::Filters::GeoIP < LogStash::Filters::Base
  # of the same geoip_type share the same cache. The last declared cache size will 'win'. The reason for this is that there would be no benefit
  # to having multiple caches for different instances at different points in the pipeline, that would just increase the
  # number of cache misses and waste memory.
-  config :lru_cache_size, :validate => :number, :default => 1000
+  config :lru_cache_size, :validate => :number, :default => 1000, :deprecated => "This field has been deprecated in favor of cache_size."


we were never using this, and it was lingering around

robcowart

Just a little feedback on field naming.

As a user the field names anonymous_system_number and anonymous_system_organization seem unnecessary long. Imagine typing those into the filter bar in Kibana. Simply using 'asn' and 'as_org' would seem sufficient. Pretty much everyone who would care what about these values uses the term ASN. Afterall, the field 'isp' isn't named 'internet_service_provider', so why name the AS-related fields in long form?

That said, if the fields are named that way within the MaxMind database and the goal is to stick with that naming, I can understand that reasoning too.

jordansissel · 2017-04-20T06:35:57Z

src/main/java/org/logstash/filters/Fields.java

+import java.util.Locale;
+
+enum Fields {
+  ANONYMOUS_SYSTEM_NUMBER("anonymous_system_number"),


In ASN, the A is autonomous, not anonymous

jordansissel · 2017-04-20T06:40:50Z

lib/logstash/filters/geoip.rb

-                                                    'dma_code', 'ip', 'latitude',
-                                                    'longitude', 'postal_code', 'region_name',
-                                                    'region_code', 'timezone', 'location']
+  config :fields, :validate => :array


Not listing a :default here makes the documentation generator (which will change soon with @ph's work) show that the setting has no default value. Could be confusing.

However, I think given @ph/@dede's efforts to change how plugins get documented, this is probably a fine change.

(Mostly noting this for posterity)

jordansissel · 2017-04-20T06:43:23Z

src/main/java/org/logstash/filters/GeoIpFilter.java

+import java.net.UnknownHostException;
+import java.util.*;
+
+public class GeoIpFilter {


IP is capitalized, and writing it "Ip" feels wrong. I dont' think we have a firm style guide on this, but my practice has always been acronyms stay all caps.

This kind of mixing gets confusing when, for example, Java crypto calls it "SSL" but Netty calls it "Ssl" ...

Do we have a practice for this?

yeah i was debating this. I do like acronyms as capitalized. I'll change it to GeoIP

jordansissel · 2017-04-20T06:44:26Z

src/main/java/org/logstash/filters/GeoIpFilter.java

+
+  private Set<Fields> createDesiredFields(List<String> fields) {
+    Set<Fields> desiredFields = EnumSet.noneOf(Fields.class);
+    if (fields == null) {


also worth checking if fields.isEmpty() ? Or would we reject an empty fields list at configuration-time?

jordansissel · 2017-04-20T06:45:52Z

src/main/java/org/logstash/filters/GeoIpFilter.java

+    }
+    // only do event.set(@target) if the lookup result is not null
+    if (event.getField(targetField) == null) {
+      event.setField(targetField, Collections.emptyMap());


is this necessary? event.set("[foo][bar]", ...) should create any parent fields if it doesn't exist... I think?

no it doesn't. I had to put this in, otherwise, tests fail. It was in the old ruby code as well.

This is because of the canceled events bug found in logstash-plugins/logstash-filter-date#90

suyograo · 2017-04-20T16:56:05Z

@robacj suggestion accepted. I shortened the fields like you mentioned.

@jordansissel updated after your comments.

oschwald · 2017-04-21T03:11:43Z

src/main/java/org/logstash/filters/GeoIPFilter.java

+public class GeoIPFilter {
+  private static Logger logger = LogManager.getLogger();
+  private static final String CITY_DB_TYPE = "GeoLite2-City";
+  private static final String COUNTRY_DB_TYPE = "GeoLite2-Country";


The commercial databases have the type GeoIP2-City and GeoIP2-Country. It'd likely be worth supporting those too, especially given that they are mentioned in the recent blog post.

@oschwald good call. Updated.

jordansissel

Overall LGTM. <3

joewreschnig · 2017-04-25T08:40:07Z

src/main/java/org/logstash/filters/GeoIpFilter.java

+
+    // if location is empty, there is no point populating geo data
+    // and most likely all other fields are empty as well
+    if (location.getLatitude() == null && location.getLongitude() == null) {


Since you're rewriting this anyway, please don't keep this mistake. There are GeoIP records with no lat-lon that this plugin should be able to handle.

acchen97 · 2017-04-26T20:15:26Z

Minor note, changelog is missing for this release.

zcola · 2017-05-18T03:48:09Z

When can I use it?

hackery · 2017-06-22T17:04:24Z

@zcola you can use it now by updating the plugin within your current logstash:

> logstash-plugin update logstash-filter-geoip
Updating logstash-filter-geoip
Updated logstash-filter-geoip 4.0.4 to 4.1.1

zcola · 2017-06-23T08:19:15Z

@hackery
I upgraded to 4.2.0 no show isp,, here is my compilation script fragment

  subdivisions        => [ 'array', 'map'],
    traits        =>  'map',
    location            => 'map',
...
my $tree = MaxMind::DB::Writer::Tree->new(
    ip_version    => 4,
    record_size   => 32,
    database_type => 'GeoIP2-ISP',
    languages     => ['en'],
...


        en      => $arr[14],
                            zh_CN   => $arr[15],
                        },
                    },
                    traits    => {
                            isp   => $arr[17],
                            organization  => $arr[19],
                    },

logstash return

{
"@timestamp" => 2017-06-23T08: 01: 36.884Z,
"Message_geo" => {
"City_name" => "Guangzhou",
"Time zone" => "Asia / Shanghai"
"Ip" => "1.0.8.0"
"Latitude" => 23.1167,
"Country_name" => "China"
"Country_code2" => "CN",
"Continent_code" => "AS",
"Country_code3" => "CN",
"Region_name" => "Guangdong",
"Location" => {
"Lon" => 113.25,
"Lat" => 23.1167
},
"Region_code" => "44"
"Longitude" => 113.25
},
"@version" => "1"
"Host" => "xxx"

Use python to parse the return

 }
    },
    "traits": {
        "organization": "\u58a8\u5c14\u672cGoldenit\u6709\u9650\u516c\u53f8",
        "isp": ""
    },
    "subdivisions": [
        {
            "iso_code": "VIC",
            "names": {
                "en": "Victoria",
                "zh_CN": ""
            }
        }

Is my compiler script wrong?

suyograo added 2 commits April 17, 2017 20:47

first stab

fad9c17

Move some parts to Java since we use Java API anyways

8784156

This was referenced Apr 20, 2017

GeoIP and ISP/ASN Support Plan #110

Closed

Fixes #91 - Allow using the ISP database #94

Closed

suyograo requested a review from jordansissel April 20, 2017 00:17

suyograo commented Apr 20, 2017

View reviewed changes

robcowart reviewed Apr 20, 2017

View reviewed changes

jordansissel suggested changes Apr 20, 2017

View reviewed changes

suyograo added 2 commits April 20, 2017 09:45

rename to GeoIP, shorten field names

6332da1

revert back to using filter() instead of multifilt

afd0a59

This is because of the canceled events bug found in logstash-plugins/logstash-filter-date#90

Actually rename file

7dc99bf

oschwald reviewed Apr 21, 2017

View reviewed changes

Support both free and commercial DBs

0c39d04

jordansissel approved these changes Apr 24, 2017

View reviewed changes

suyograo merged commit ec18789 into logstash-plugins:master Apr 24, 2017

suyograo deleted the java_this branch April 24, 2017 17:56

joewreschnig reviewed Apr 25, 2017

View reviewed changes

acchen97 mentioned this pull request Apr 26, 2017

Add support for GeoIP2-ISP database #91

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support ISP database for ASN data and move parts to Java #111

Support ISP database for ASN data and move parts to Java #111

suyograo commented Apr 20, 2017

suyograo Apr 20, 2017

jordansissel Apr 20, 2017

suyograo Apr 20, 2017

robcowart left a comment

jordansissel Apr 20, 2017

jordansissel Apr 20, 2017

jordansissel Apr 20, 2017

suyograo Apr 20, 2017

jordansissel Apr 20, 2017

jordansissel Apr 20, 2017

suyograo Apr 20, 2017

suyograo commented Apr 20, 2017

oschwald Apr 21, 2017

suyograo Apr 21, 2017

jordansissel left a comment

joewreschnig Apr 25, 2017

acchen97 commented Apr 26, 2017

zcola commented May 18, 2017

hackery commented Jun 22, 2017

zcola commented Jun 23, 2017

Support ISP database for ASN data and move parts to Java #111

Support ISP database for ASN data and move parts to Java #111

Conversation

suyograo commented Apr 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robcowart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suyograo commented Apr 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordansissel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acchen97 commented Apr 26, 2017

zcola commented May 18, 2017

hackery commented Jun 22, 2017

zcola commented Jun 23, 2017