[GitHub] metron issue #827: METRON-1294: IP addresses are not formatted correctly in ...
Github user justinleet commented on the issue: https://github.com/apache/metron/pull/827 Great, thanks for the update. +1 ---
[GitHub] metron issue #827: METRON-1294: IP addresses are not formatted correctly in ...
Github user justinleet commented on the issue: https://github.com/apache/metron/pull/827 @nickwallen The parameter name is preexisting and I'm fine with leaving that. Would you be okay with changing the descriptions in the README and annotations? That should be a nonintrusive change that doesn't require spinning everything back up. @merrimanr Do you have any objections to doing that as a middle ground? ---
[GitHub] metron issue #827: METRON-1294: IP addresses are not formatted correctly in ...
Github user nickwallen commented on the issue: https://github.com/apache/metron/pull/827 @justinleet I think that's a good find. I'd suggest we fix this issue on a subsequent PR after we get this and #832 merged. On #832, the refactoring will make it much easier to unit test a fix for this condition. Let me know if you think that works or if you think its critical on this PR. ---
[GitHub] metron issue #827: METRON-1294: IP addresses are not formatted correctly in ...
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/827
I ran a request giving sensors:
```
curl -X POST --header 'Content-Type: application/json' --header 'Accept:
application/json' -d '["snort", "bro"]'
'http://node1:8082/api/v1/search/column/metadata'
```
which returns fine
```
{
"TTLs": "double",
"bro_timestamp": "string",
"enrichments:geo:ip_dst_addr:location_point": "other",
"sha256": "string",
"enrichmentjoinbolt:joiner:ts": "date",
"certificate:version": "integer",
...
}
```
but giving the actual indices returns nothing, e.g.
```
curl -X POST --header 'Content-Type: application/json' --header 'Accept:
application/json' -d '["bro_index_2017.11.17.14"]'
'http://node1:8082/api/v1/search/column/metadata'
```
I assume it's intentional that indices don't actually return data, which
I'm fine with, but we need to rename things from indices to sensor or something.
---
[GitHub] metron issue #827: METRON-1294: IP addresses are not formatted correctly in ...
Github user merrimanr commented on the issue: https://github.com/apache/metron/pull/827 The latest commit fixes the bug @justinleet found and adds some test cases to cover it. I also removed an "ignoredIndices" variable from the ElasticsearchDao class. This was being used to exclude certain indices but I believe we've moved towards a white list approach for specifying indices so this no longer makes sense. I can revert if anyone disagrees. ---
[GitHub] metron issue #827: METRON-1294: IP addresses are not formatted correctly in ...
Github user merrimanr commented on the issue: https://github.com/apache/metron/pull/827 @JonZeolla I think we should just remove it for now and I've done that in the latest commit. I'm not entirely clear on what we should expect in this case and we don't need it right now anyways. Better to implement it the right way once we are clear on how it should behave in my opinion. Good catch @justinleet! I did not think of this test case and to my surprise the Elasticsearch API returns ALL field mappings if you pass in an empty array of indices. I will fix this and add a test case. You are correct in that we should expect correctly formatted ip addresses in "facetCounts". Before this PR they would have been in long format when you include an index that does not have a mapping for the ip_src_addr field. ---
[GitHub] metron issue #827: METRON-1294: IP addresses are not formatted correctly in ...
Github user justinleet commented on the issue: https://github.com/apache/metron/pull/827 I tried hitting the `/api/v1/search/column/metadata` endpoint on fulldev with `["madeupindex"]`; e.g. curl -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '["madeupindex"]' 'http://node1:8082/api/v1/search/column/metadata' I would expect this to return no results, because the index doesn't exist, but instead I get back a lot of fields. ---
[GitHub] metron issue #827: METRON-1294: IP addresses are not formatted correctly in ...
Github user JonZeolla commented on the issue: https://github.com/apache/metron/pull/827 I didn't get myself intimately familiar with this PR, but I wanted to mention that assuming two fields with the same name but different types between indexes are not the same may not always hold. For instance, in bro there are a couple of field names that overlap between bro logs, and so the type in the bro index needs to be one that can commonly work (usually string), but if there was no overlap it would be a different type. [Example](https://github.com/JonZeolla/metron/blob/master/metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/package/files/bro_index.template#L131-L134). Not even positive this is relevant (especially given `getCommonColumnMetadata` may just get removed), but in case it is I wanted to mention it. ---
[GitHub] metron issue #827: METRON-1294: IP addresses are not formatted correctly in ...
Github user merrimanr commented on the issue: https://github.com/apache/metron/pull/827 Was commenting while your comment posted. I think your latest suggestion is the right answer :) ---
[GitHub] metron issue #827: METRON-1294: IP addresses are not formatted correctly in ...
Github user merrimanr commented on the issue: https://github.com/apache/metron/pull/827 Actually the more I think about it, I would not consider a field to be common between indices when the types mismatch. Hard to say what the right answer is because there is no real requirement driving this. ---
[GitHub] metron issue #827: METRON-1294: IP addresses are not formatted correctly in ...
Github user nickwallen commented on the issue: https://github.com/apache/metron/pull/827 Even better then. Can we just get rid of it? ---
[GitHub] metron issue #827: METRON-1294: IP addresses are not formatted correctly in ...
Github user merrimanr commented on the issue: https://github.com/apache/metron/pull/827 I don't think that method is even used anymore but I will add the mismatch handling just in case. ---
[GitHub] metron issue #827: METRON-1294: IP addresses are not formatted correctly in ...
Github user nickwallen commented on the issue: https://github.com/apache/metron/pull/827 How does `getCommonColumnMetadata` behave when we have a mismatched field (aka a field named the same in two different indices, but with different types)? Do we need similar mismatch handling? ---
[GitHub] metron issue #827: METRON-1294: IP addresses are not formatted correctly in ...
Github user nickwallen commented on the issue: https://github.com/apache/metron/pull/827 I submitted a PR against branch that adds a specific test case in the integration tests for this defect. I validated that this test fails in master, but passes with your fix. Feel free to include in your PR. ---
