-
Notifications
You must be signed in to change notification settings - Fork 512
METRON-2306: [UI] Grok parsers' have duplicate timestampField #1553
base: master
Are you sure you want to change the base?
Conversation
@@ -23,7 +23,7 @@ export class SensorParserConfig { | |||
writerClassName: string; | |||
errorWriterClassName: string; | |||
invalidWriterClassName: string; | |||
parserConfig: {}; | |||
parserConfig: { [key: string]: any; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typing using {}
just extends the Object interface. What we really want here is an object literal that's editable by users.
@@ -191,8 +192,8 @@ export class SensorParserConfigComponent implements OnInit { | |||
|
|||
ngOnInit() { | |||
this.route.params.subscribe(params => { | |||
let id = params['id']; | |||
this.init(id); | |||
this.paramsID = params['id']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Storing the id in a class property allows me to use it when evaluating whether or not to populate the parserConfig object with the timestampField property and default timestamp
value.
@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. |
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:
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:
That first parameter in FormControl is what a field's default value should be. It should be looking at
That's exactly what I implemented in this PR.
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. |
@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. |
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: #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. IftimestampField
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:
For code changes:
Have you included steps to reproduce the behavior or problem that is being changed or addressed?
Have you included steps or a guide to how the change may be verified and tested manually?
Have you ensured that the full suite of tests and checks have been executed in the root metron folder via:
Have you written or updated unit tests and or integration tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
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:
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
: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.