-
Notifications
You must be signed in to change notification settings - Fork 467
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
bitdefender: Add jsonRPC format as recommended default. #10381
Conversation
Pinging @elastic/security-service-integrations (Team:Security-Service Integrations) |
{{else}} | ||
program: | | ||
has(obj.jsonrpc) ? | ||
dyn({ | ||
"error": { | ||
"message": "Unable to process message. Received jsonrpc formatted message, but setting `Enable jsonRPC Format` is disabled." | ||
} | ||
}) | ||
: | ||
obj |
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.
I'm not sure about this part; the CEL processing is more expensive than the direct publication. Do you think that the error message here carries its weight beyond what would be seen without? A possible alternative seems like it should be possible in the ingest pipeline; test there for the presence of the json.jsonrpc
field and add this same message then. The difference would be that the agent doesn't know about it. This difference may be significant when the status updates are added to the http_endpoint input.
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.
I ended up removing the user option to enable/disable the jsonrpc
format. The input now parses all 3 formats jsonrpc
, qradar
or splunk
without any fail
processors in ingest pipeline. Change in 5867fdf. Let me know if this sounds good.
...sh_notifications/_dev/test/system/test-bitdefender-push-notification-jsonrpc-http-config.yml
Show resolved
Hide resolved
- name: enable_jsonrpc_format | ||
type: bool | ||
title: Enable jsonRPC Format. | ||
description: Enable this flag if events are in BitDefender's jsonRPC format. For details, see [documentation](https://www.bitdefender.com/business/support/en/77209-135325-push-event-json-rpc-messages.html). |
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.
The text here says "enable this … if", but it is already on by default. Suggest "Enable processing events in BitDefender's jsonRPC format."?
Do we want this option on by default?
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.
I will change the description. Initially I added it as default: false
and its description accordingly.
But after talking to Bitdefender folks, they recommend using this format as default. So, I changed the default: true
and incremented major version to indicate this is a breaking change. I forgot to update the description accordingly.
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.
Removed this option all together
#10381 (comment)
packages/bitdefender/data_stream/push_notifications/elasticsearch/ingest_pipeline/default.yml
Show resolved
Hide resolved
packages/bitdefender/data_stream/push_notifications/elasticsearch/ingest_pipeline/default.yml
Show resolved
Hide resolved
🚀 Benchmarks reportPackage
|
Data stream | Previous EPS | New EPS | Diff (%) | Result |
---|---|---|---|---|
push_configuration |
29411.76 | 23809.52 | -5602.24 (-19.05%) | 💔 |
To see the full report comment with /test benchmark fullreport
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.
LGTM after nits.
💚 Build Succeeded
History
|
Quality Gate passedIssues Measures |
Package bitdefender - 2.0.0 containing this change is available at https://epr.elastic.co/search?package=bitdefender |
Add support for `jsonRPC` format. After input limitation in processing `jsonRPC` is removed, this PR adds support to process `jsonRPC` formatted events and also suggests making this as default format as recommended by BitDefender. - Update docs to indicate `jsonRPC` is recommended default. - Increment major version to support this breaking change making jsonRPC as default recommended.
Proposed commit message
Checklist
changelog.yml
file.Author's Checklist
How to test this PR locally
$ eval "$(elastic-package stack shellinit)" && elastic-package test pipeline --generate -v --data-streams=push_notifications
$ eval "$(elastic-package stack shellinit)" && elastic-package test system --generate -v --data-streams=push_notifications
Related issues
Screenshots