tiborm commented on issue #1431: METRON-2102: [UI] Adding click-through navigation to Alerts table URL: https://github.com/apache/metron/pull/1431#issuecomment-498642471 > @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. I agree with @sardell on this. The isEnable config parameter here is not a feature flag. IMHO feature flags are for development teams. In this case, the isEnabled parameter is for our end users (more precisely for those who maintain deployed Metron on a service level). Even if the click through navigation was put behind a feature flag, I would keep the isEnabled flags for our clients to turn on and off based according to their preference. Having this feature on by default would weaken the user experience if users don’t want to use it. I know you already agreed with @sardell briefly pointing this out, but I wanted to clarify my use of the isEnabled parameter. On a related note, I added these two followup Jira here: [Create REST endpoint to persist Click Through navigation configuration](https://issues.apache.org/jira/browse/METRON-2104) [Impl Click Through navigation admin interface on Management UI](https://issues.apache.org/jira/browse/METRON-2103) With these two additional changes, isEnabled would be fully exposed to our end users (trough a toggle control) and they would be able to turn it on or off through the Management UI. > 2. 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? If the field name is misspelled then the isEnabled field is missing. It will evaluate as not true, therefore the feature stays turned off. I’ll extend the implementation with warnings in the console log if any required configuration is missing. I’m also going to cover these scenarios with unit tests. If a field name is misspelled it will be considered missing. None of these scenarios causing errors but I’ll cover them with unit tests. > 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? That would actually evaluate as true because of Javascript’s auto type coercion. I’m going to fix this and cover it with unit tests. > 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? Visibility of a column has no impact on the token replacement. Referencing a non-existing token causes no error. However, the URL will not be resolved properly. I’ll cover this scenario with unit tests as well. And add log messages to help configuration. > 3. 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) > 2. 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) Will do. This should be easy using the description in Jira as a basis. Thanks @mmiklavc
---------------------------------------------------------------- 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: us...@infra.apache.org With regards, Apache Git Services
