Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 'summary_table_max_rows' configuration option #508

Merged
merged 8 commits into from
Oct 13, 2021

Conversation

ghost
Copy link

@ghost ghost commented Oct 12, 2021

Description

There is currently no way of limiting the number of rows in a summary_table. This can lead to very long "summary" tables that can break formatting when sending notifications.

Checklist

  • I have reviewed the contributing guidelines.
  • I have included unit tests for my changes or additions.
  • I have successfully run make test-docker with my changes.
  • I have manually tested all relevant modes of the change in this PR.
  • I have updated the documentation.
  • I have updated the changelog.

Questions or Comments

This is a small code change. I've been running this for a few days, there have been no issues.

@nsano-rururu
Copy link
Collaborator

@mdavyt92

Does that mean adding a setting called summary_table_max_rows? .. If so, I think the documentation needs to be modified.
Also, schema.yaml
https://github.com/jertel/elastalert2/blob/master/docs/source/ruletypes.rst
https://github.com/jertel/elastalert2/blob/master/elastalert/schema.yaml

@nsano-rururu
Copy link
Collaborator

Please also update the change log.

@ghost
Copy link
Author

ghost commented Oct 13, 2021

Updated documentation and changelog. Not sure what to do with the schema

@nsano-rururu
Copy link
Collaborator

Updated documentation and changelog. Not sure what to do with the schema

Is it a character? Is it a number? If it is a number, is there a range that can be set? .. Does that mean that decimal points are not allowed?

@ghost
Copy link
Author

ghost commented Oct 13, 2021

It's a non negative integer.
I searched for summary_table_fields or summary_table_type in the schema to use as template, but I found nothing.

@nsano-rururu
Copy link
Collaborator

nsano-rururu commented Oct 13, 2021

It's a non negative integer. I searched for summary_table_fields or summary_table_type in the schema to use as template, but I found nothing.

The reason why there is no setting. Dunno. I didn't make it.
Since the validation check setting itself was added later, there is no problem in itself that there is no setting.
Also add the following settings above the ### Kibana Discover App Link.
Check the operation locally and make a pull request if there is no problem when executing the rule.

### summary table
summary_table_fields: {type: array, items: {type: string}}
summary_table_type: {type: string, enum: ['ascii', 'markdown']}
summary_table_max_rows: {type: number}
summary_prefix: {type: string}
summary_suffix: {type: string}

@nsano-rururu
Copy link
Collaborator

@jertel

Is there anything else you care about?

@jertel
Copy link
Owner

jertel commented Oct 13, 2021

@mdavyt92 Please add a unit test to alerts_test.py for this new option. You can use test_alert_aggregation_summary_default_table as a template and configure the new summary_table_max_rows setting to limit it to 1 row, then assert that the cde from match row doesn't exist in the output summary_table variable.

Copy link
Owner

@jertel jertel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the new summary table max rows param!

@jertel jertel merged commit 0a51b17 into jertel:master Oct 13, 2021
@nsano-rururu
Copy link
Collaborator

@jertel

I made a mistake in pointing out the review. I'm sorry. With the current settings, negative and decimal values are also allowed. Correctly, you need to modify schema.yaml as follows.

summary_table_max_rows: {type: number}

        ↓

summary_table_max_rows: {type: integer, 'minimum': 0}

@jertel
Copy link
Owner

jertel commented Oct 13, 2021

Thanks for the heads-up @nsano-rururu. I've pushed the change directly to master: 0ec9179

I don't think the single quotes are required since other schema.yaml lines did not specify 'minimum': but rather minimum:.

@nsano-rururu
Copy link
Collaborator

@jertel

surely. No single quotes required. We apologize for including unnecessary information.
In addition, the operation check of json schema was done with the following code.

import jsonschema

schema = {
     "type" : "object",
     "properties" : {
         "price" : {"type" : "integer", "minimum": 0},
         "name" : {"type" : "string"},
         "to" : {"type" : "string", "format": "email"},
         "url" : {"type" : "string", "format": "uri"}
     },
 }

try:
    jsonschema.validate(instance={"name" : "1", "price" : 1, "to": "aaa@abc.com", "url" : "100"}, schema=schema, format_checker=jsonschema.FormatChecker())
except Exception as e:
    print(e)

price is 1

(myvenv) [CORP\sano@a-ngft53r34ong pywork]$ python json2.py 

price is -1

(myvenv) [CORP\sano@a-ngft53r34ong pywork]$ python json2.py 
-1 is less than the minimum of 0

Failed validating 'minimum' in schema['properties']['price']:
    {'minimum': 0, 'type': 'integer'}

On instance['price']:
    -1

price is 0.2

(myvenv) [CORP\sano@a-ngft53r34ong pywork]$ python json2.py 
0.2 is not of type 'integer'

Failed validating 'type' in schema['properties']['price']:
    {'minimum': 0, 'type': 'integer'}

On instance['price']:
    0.2

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants