METRON-2306: [UI] Grok parsers' have duplicate timestampField

[sardell]

Contributor Comments

Link to original ASF Jira ticket: https://issues.apache.org/jira/browse/METRON-2306

Currently, when a user opens a Grok parser config in the Management UI, they are presented with two inputs for the timestampField field. The first is an input labelled ‘Timestamp’:

This input was added recently because Grok parsers require you to have a timestampField attribute. You can read more about it here: https://github.com/apache/metron/pull/1393

The second is under the Parser Config section of the side panel:

This section is a form representation of the parserConfig property that is returned from the REST API once a parser exists. It is fully editable by the user.

Naturally, some confusion has come up around where a user manages this field. The first mentioned field defaults to ‘timestamp’, but does not populate a value on an existing parser since that value is returned from the API in the parserConfig object.

In this PR, I’ve removed the first mentioned field altogether, and instead upon creation I’m defaulting to the field value ‘timestamp’ in the parserConfig object. If a user tries to remove the timestampField from the config object, the submit button becomes disabled. Unfortunately, adding a validation message is not something I've done yet. This would require more work due to the current nested structure of the parserConfig form. Before I continue with this work, I think we should actually discuss a better solution.

I think the best way to fix this is to actually change the backend so that Grok parsers have and require a property for timestampField. I think this because, as mentioned above, the current implementation in the UI allows users to create, update and delete any property in the parserConfig object. If timestampField is actually a requirement for Grok parsers, I do not think this property should be part of this object, as it is currently.

Pull Request Checklist

Thank you for submitting a contribution to Apache Metron.
Please refer to our Development Guidelines for the complete guide to follow for contributions.
Please refer also to our Build Verification Guidelines for complete smoke testing guides.

In order to streamline the review of the contribution we ask you follow these guidelines and ask you to double check the following:

For all changes:

• [x] Is there a JIRA ticket associated with this PR? If not one needs to be created at Metron Jira.
• [x] Does your PR title start with METRON-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
• [x] Has your PR been rebased against the latest commit within the target branch (typically master)?

For code changes:

• [x] Have you included steps to reproduce the behavior or problem that is being changed or addressed?

• [x] Have you included steps or a guide to how the change may be verified and tested manually?

• [x] Have you ensured that the full suite of tests and checks have been executed in the root metron folder via:

  mvn -q clean integration-test install && dev-utilities/build-utils/verify_licenses.sh 
  

• [x] Have you written or updated unit tests and or integration tests to verify your changes?

• [x] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?

• [x] Have you verified the basic functionality of the build by building and running locally with Vagrant full-dev environment or the equivalent?

For documentation related changes:

• [x] Have you ensured that format looks appropriate for the output in which it is rendered by building and verifying the site-book? If not then run the following commands and the verify changes via site-book/target/site/index.html:

  cd site-book
  mvn site
  

• [x] Have you ensured that any documentation diagrams have been updated, along with their source files, using draw.io? See Metron Development Guidelines for instructions.

Note:

Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible. It is also recommended that travis-ci is set up for your personal repository such that your branches are built there before submitting a pull request.

[mmiklavc]

@sardell - I'm a little confused - the only place I think we deal with timestamp fields and/or properties is in the parserConfig itself (the very specifically named object within the broader parser configuration - yeah, calling something a "parserConfig" within a parser config is a bit confusing to try to discuss accurately). Anyhow, where is/was the other "TIMESTAMP" property being set if not in the "parserConfig" object? Specifically for Grok parsers, (e.g. Yaf, Squid) isn't this just a matter of requiring the "timestampField" property in the config section?

It's possible this is a problematic request bc of how that parserConfig object is handled in the UI, but you might also consider making "TIMESTAMP" readonly and simply grab the value from "timestampField", with the error message reflecting the same. I'm not sure I completely understand the problem in the UI, so maybe we can discuss this a bit further.

[sardell]

@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 an input field separate from the parserConfig section.

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.

[sardell]

@mmiklavc I managed to update the parserConfig to include a validation message for the timestampField. Whenever you get a chance, take a look and let me know if this is an acceptable solution for you, and if I cleared up the questions you had with my previous comment.