mmiklavc commented on issue #1431: METRON-2102: [UI] Adding click-through navigation to Alerts table URL: https://github.com/apache/metron/pull/1431#issuecomment-497794088 @tiborm this looks like a pretty cool feature, thanks for the contribution! I'd like to address a couple things before we commit this. I am on board with providing a flag for this feature and leaving it disabled by default - that looks reasonable to me, and I also coincidentally just kicked off a DISCUSS thread along these lines as well - https://lists.apache.org/thread.html/e24e4709fbc06473977a0a7f0c0ce92eb02cfcc8c1c2bc41be08a698@%3Cdev.metron.apache.org%3E. I do think we need to resolve some outstanding questions about how we handle that as a community - see below. I looked over your instructions on the Jira ticket and I think we're pretty close. I'd like to see the following: 1. What's the plan for handling this feature flag? We haven't had any discussion in the community about enabling feature flags to this point. I suspect we might consider them an option for lowering the bar to testing, but we should address this. I don't see any non-happy path tests on this PR, for example. I think that either we need to put this PR through more rigorous testing or come to a community understanding (and document it in the dev guidelines) about what a reasonable level of quality and feature polish will be for code committed under a feature flag. 1. Non-happy path test scenarios? e.g. 1. What happens to the UI if you misspell a field name? What errors/indication does the user get? Will the UI fail to come up? Will it only fail when a field is visible in the UI column listing? 2. What happens if you type "fasle" for `isEnabled`? Again, what's the default behavior for misconfiguration and where can operations people managing this feature look for logs that detail the exceptions? 3. What happens for fields not present in a particular record that are used for token replacement? e.g. per the Jira instructions - "But in the configuration, any available alert property field could be referenced like the following:" Also, is this impacted in any way by what fields are currently displayed/enabled in the alerts UI column listing? If I've hidden ip_src_addr from display, will it still render the URL correctly? 1. These instructions should have a dedicated section in the documentation/README for the Alerts UI. 1. Please provide a detailed explanation of the configuration lifecycle - will the changes be picked up automatically, or only after a restart? Provide instructions for whatever those steps are (e.g. restart Alerts UI using command foo) 1. Provide a detailed schema description for the JSON you provided in your testing examples. I think you've done a decent job of explaining what `alertEntry` and `metaAlertEntry` mean, this just needs to be expanded on a bit and laid out similar to what we've done for parsers here - [general purpose parser config options](https://github.com/apache/metron/tree/master/metron-platform/metron-parsing#introduction) and here - [config details for all parsers](https://github.com/apache/metron/tree/master/metron-platform/metron-parsing#parser-configuration)
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
