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.

Sign up for GitHub

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

Jump to bottom

Parameterizing Painless script literals #9770

Merged
merged 11 commits into from
Dec 28, 2018
Merged

Parameterizing Painless script literals #9770

merged 11 commits into from
Dec 28, 2018

Conversation

ycombinator
Copy link
Contributor

@ycombinator ycombinator commented Dec 22, 2018 •
edited
Loading

Several Filebeat CI runs are failing like so:

05:55:30 ======================================================================
05:55:30 FAIL: test_fileset_file_7_traefik (test_modules.Test)
05:55:30 ----------------------------------------------------------------------
05:55:30 Traceback (most recent call last):
05:55:30   File "/go/src/github.com/elastic/beats/filebeat/build/python-env/local/lib/python2.7/site-packages/parameterized/parameterized.py", line 392, in standalone_func
05:55:30     return func(*(a + p.args), **p.kwargs)
05:55:30   File "/go/src/github.com/elastic/beats/filebeat/tests/system/test_modules.py", line 91, in test_fileset_file
05:55:30     cfgfile=cfgfile)
05:55:30   File "/go/src/github.com/elastic/beats/filebeat/tests/system/test_modules.py", line 140, in run_on_file
05:55:30     obj)
05:55:30 AssertionError: not error expected but got: {u'http': {u'version': u'1.0', u'request': {u'method': u'GET'}, u'response': {u'status_code': 200}}, u'log': {u'file': {u'path': u'/go/src/github.com/elastic/beats/filebeat/module/traefik/access/test/test.log'}, u'offset': 1581}, u'url': {u'original': u'/apache_pb.gif'}, u'traefik': {u'access': {u'body_sent': {u'bytes': 2326}, u'user_identifier': u'-'}}, u'@timestamp': u'2000-10-10T20:55:36.000Z', u'read_timestamp': u'2018-12-22T13:50:36.804Z', u'agent': {u'hostname': u'47c93834e587', u'type': u'filebeat', u'version': u'7.0.0'}, u'source': {u'ip': u'127.0.0.1', u'address': u'127.0.0.1'}, u'host': {u'name': u'47c93834e587'}, u'user': {u'name': u'frank'}, u'error': {u'message': u'[script] Too many dynamic script compilations within, max: [75/5m]; please use indexed, or scripts with parameters instead; this limit can be changed by the [script.max_compilations_rate] setting'}, u'input': {u'type': u'log'}, u'event': {u'module': u'traefik', u'dataset': u'access'}}
05:55:30 -------------------- >> begin captured stdout << ---------------------
05:55:30 Using elasticsearch: http://elasticsearch:9200
05:55:30 Testing traefik/access on /go/src/github.com/elastic/beats/filebeat/tests/system/../../module/traefik/access/test/test.log
05:55:30 
05:55:30 --------------------- >> end captured stdout << ----------------------
05:55:30 
05:55:30 ----------------------------------------------------------------------

For example:

This PR tries to parameterize any painless scripts in Filebeat modules' pipelines to see if it helps with the Too many dynamic script compilations within, max: [75/5m] error. Specifically, it parameterizes literals that were previous used in painless scripts by the following Filebeat module pipelines:

  • auditd
  • nginx
  • redis

@ycombinator
Copy link
Contributor Author

jenkins, test this

@ruflin
Copy link
Member

ruflin commented Dec 24, 2018

@ycombinator Thanks for starting to tackle this. I also made some notes about this here: #9777 (see the comments below).

To have much nicer PR's we should probably take on #5339 again. This would make things nicer.

@ycombinator
Copy link
Contributor Author

jenkins test this

@ycombinator ycombinator changed the title Parameterizing some script literals Parameterizing Painless script literals Dec 26, 2018
@ycombinator ycombinator added review needs_backport PR is waiting to be backported to other branches. v7.0.0-alpha2 labels Dec 26, 2018
@ruflin
Copy link
Member

ruflin commented Dec 27, 2018

CI failure does not seem to be related. Need to check if this sneaked into master in an other PR as it complains about traefik and ingesting data into an alias.

@ruflin
Copy link
Member

ruflin commented Dec 27, 2018

Rebasing on master should get CI green for this one.

@ruflin ruflin added the Team:Integrations Label for the Integrations team label Dec 27, 2018
@ycombinator ycombinator force-pushed the fb-pipelines-parameterize-script-literals branch from 5e431e0 to eac4f37 Compare December 27, 2018 16:52
@ycombinator
Copy link
Contributor Author

@ruflin CI is green now. When you get a chance, could you review please?

ruflin
ruflin reviewed Dec 28, 2018
View reviewed changes
filebeat/module/auditd/log/ingest/pipeline.json Outdated Show resolved Hide resolved
filebeat/module/redis/log/ingest/pipeline.json Show resolved Hide resolved
filebeat/module/redis/log/ingest/pipeline.json
"lang": "painless",
"source": "if (ctx.redis.log.role == params.master_abbrev) {\n ctx.redis.log.role = params.master;\n } else if (ctx.redis.log.role == params.slave_abbrev) {\n ctx.redis.log.role = params.slave;\n } else if (ctx.redis.log.role == params.child_abbrev) {\n ctx.redis.log.role = params.child;\n } else if (ctx.redis.log.role == params.sentinel_abbrev) {\n ctx.redis.log.role = params.sentinel;\n }\n ",
"params": {
"master_abbrev": "M",
Copy link
Member

Choose a reason for hiding this comment

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

It seems odd that we have to extract all these constants which are only used once but I don't think there is a better way. Perhaps @jakelandis can comment?

@ruflin
Copy link
Member

ruflin commented Dec 28, 2018

I created a meta issue here to track this: #9821

@ycombinator
Copy link
Contributor Author

ycombinator commented Dec 28, 2018 •
edited
Loading

@ruflin Made the changes you requested. Ready for your 👀 again, when you get a chance. Thanks!

ruflin
ruflin approved these changes Dec 28, 2018
View reviewed changes
Copy link
Member

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

As nothing changed in the golden files, I think we are good. Tricky to review TBH.

@ycombinator
Copy link
Contributor Author

jenkins, test this

1 similar comment
@ycombinator
Copy link
Contributor Author

jenkins, test this

@ycombinator ycombinator merged commit 2c6030d into elastic:master Dec 28, 2018
@ycombinator ycombinator deleted the fb-pipelines-parameterize-script-literals branch December 28, 2018 17:38
@ycombinator ycombinator mentioned this pull request Dec 28, 2018
Merged
@ycombinator ycombinator added v6.7.0 and removed needs_backport PR is waiting to be backported to other branches. labels Dec 28, 2018
ycombinator added a commit that referenced this pull request Jan 8, 2019
@ycombinator
Cherry-pick #9770 to 6.x: Parameterizing Painless script literals (#9831
a0e5c65 
)

* Parameterizing Painless script literals (#9770)

* Parameterizing some script literals

* Fixing script property name

* Fixing syntax error

* Reverting nginx change

* Reverting auditd pipeline change (for now)

* Work around painless limitation

* Working around Painless issue for nginx module ingest pipeline

* Parameterizing one more script in redis module ingest pipeline

* Adding back lang field to explicitly set script language to Painless

* Reformatting nginx access log

* Fixing indent

(cherry picked from commit 2c6030d)

* Fixing up redis pipeline for 6.x

* Fixing up nginx pipeline for 6.x
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ruflin ruflin ruflin approved these changes

@jsoriano jsoriano Awaiting requested review from jsoriano

@jakelandis jakelandis Awaiting requested review from jakelandis

Assignees
No one assigned
Labels
Filebeat Filebeat module review Team:Integrations Label for the Integrations team v6.7.0 v7.0.0-alpha2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ycombinator @ruflin