-
Notifications
You must be signed in to change notification settings - Fork 456
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
[logtash] align logstash logs ingestion #4206
Conversation
🌐 Coverage report
|
/test |
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.
Had some thoughts from initial visual review.
@@ -39,6 +43,6 @@ | |||
- name: text | |||
type: text | |||
- name: plugin_params_object | |||
type: object | |||
type: flattened |
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.
Should we update https://github.com/elastic/beats/blob/main/filebeat/module/logstash/slowlog/_meta/fields.yml#L43-L46 to match?
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.
Good catch. For context, I caught that with the system tests (they run locally but fail in CI, we'll investigate it in #4008) which fail when ingested documents have properties undefined in the mappings and in that case it was the nested properties under plugin_params_object
. Because the properties are not fixed (they're dependent on the slow plugin) I thought flattened
type would be a good use case here. We can try to map each potential nested property but that does not scale
"timeRestore": false, | ||
"title": "[Logs Logstash] Logstash Logs", | ||
"version": 1 | ||
}, | ||
"coreMigrationVersion": "7.15.0", | ||
"coreMigrationVersion": "8.5.0", |
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.
This has me questioning what happens when we start to mix kibana & package versions. Since we're kibana.version: ^8.5.0
though it should be fine here.
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.
Can you clarify the concern ?
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 imagining we might re-export dashboards from something later, say 8.9, but forget to bump the manifest version. Then it'll allow installation on 8.5 even though the dashboards probably won't import.
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 toolchain runs a test suite that ensures the assets can be installed so that will be caught by CI
Failure example
Error: error running package asset tests: could not complete test run: can't install the package: can't install the package: could not install package; API status code = 422; response body = {"statusCode":422,"error":"Unprocessable Entity","message":"Document "logstash-Logs-Logstash-Log-Dashboard" has property "dashboard" which belongs to a more recent version of Kibana [8.6.0]. The last known version is [8.5.0]"}
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.
packages/logstash/data_stream/log/elasticsearch/ingest_pipeline/default.yml
Show resolved
Hide resolved
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've noticed that on the readme, it still says that logstash is compatible with 7.3 or later. Should we change this here or on a follow-up PR?
I've updated the support version for now but we should review the readme separately |
🚀 Benchmarks reportTo see the full report comment with |
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!
Summary
Closes #4048
data_stream.dataset
property instead of the outdated prefix filter. Since dashboard and visualizations rely on the saved search it is the only asset that was updated functionally, but I also re-generated the other ones so we have them in the latest formatThe service is set to output the logs in json, I've also verified that plaintext logs were successfully ingested and added test cases for both json and plain. You can comment this line to test plaintext.
Notes
Testing
(cd packages/logstash && elastic-package build -v)
elastic-package stack up -v -d --version 8.5.0-SNAPSHOT
(cd packages/logstash && elastic-package service up -v)
/tmp/service_logs/logstash/logstash-json.log
and/tmp/service_logs/logstash/logstash-slowlog-json.log
.logstash.log
andlogstash.slowlog
datasets are ingested