[GitHub] [metron] sardell commented on issue #1553: METRON-2306: [UI] Grok parsers' have duplicate timestampField

[GitBox <gi...@apache.org>]

sardell commented on issue #1553: METRON-2306: [UI] Grok parsers' have duplicate timestampField
URL: https://github.com/apache/metron/pull/1553#issuecomment-550058047
 
 
   @mmiklavc 
   > Anyhow, where is/was the other "TIMESTAMP" property being set if not in the "parserConfig" object?
   
   When you fill in the TIMESTAMP input (ie. the input outside of the parser config object UI area) and submit to create a new parser, the request looks like this:
   ```
   {
       "parserConfig": {
           "grokPath": "/apps/metron/patterns/test",
           "patternLabel": "TEST"
       },
       "fieldTransformations": [],
       "spoutConfig": {},
       "stormConfig": {},
       "timestampField": "timestamp",
       "parserClassName": "org.apache.metron.parsers.GrokParser",
       "sensorTopic": "yaf"
   }
   ```
   
   That's because the model we have implemented on the front-end expects timestampField to be a property of the parser, but instead it's a property of parserConfig. This also explains why we do not see the value of an existing parser in that field. The validator for that input looks like this:
   
   ```
   group['timestampField'] = new FormControl(
         this.sensorParserConfig.timestampField,
         Validators.required
       );
   ```
   That first parameter in FormControl is what a field's default value should be. It should be looking at `this.sensorParserConfig.parserConfig.timestampField` instead given what is currently returned. I 
    haven't looked into whether this is the result of miscommunication at implementation or a change in what the REST endpoint is returning, but this is why it currently doesn't work.
   
   > Specifically for Grok parsers, (e.g. Yaf, Squid) isn't this just a matter of requiring the "timestampField" property in the config section?
   
   That's exactly what I implemented in this PR.
   
   > you might also consider making "TIMESTAMP" readonly and simply grab the value from "timestampField", with the error message reflecting the same
   
   I could do this. Given the way we currently present the parserConfig object in the UI, I felt it was better to keep the property in the parserConfig. However, I can be swayed to take this approach if others feel it's better to have a separate input field.
   
   When I made this PR, I made a lot of assumptions based on what is implemented in the UI. This is why I originally mentioned that the REST API should be updated to have timestampField as a property on the Grok parser itself. However, if it belongs inside the parserConfig object, I would rather include it as part of the parserConfig section in the UI. Unless anyone objects, I'm going to proceed to add validation onto the work I did and update here when I have that done.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services