-
Notifications
You must be signed in to change notification settings - Fork 465
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
[elasticsearch] add static and pipeline tests #4122
Conversation
🌐 Coverage report
|
This reverts commit 075372a.
packages/elasticsearch/data_stream/slowlog/elasticsearch/ingest_pipeline/default.yml
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,201 @@ | |||
{ | |||
"expected": [ |
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.
Just a note: In real world, the expected value will contain fields added by the some filebeat processors configured here. One example is the ecs.version
, which is not present here because the pipeline tests don't consider filebeat processors config.
About the ecs.version
field, now looking at the hardcoded value, I wonder whether it's correct. ECS version is now on 8.4. Keeping this up-to-date will be tough
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.
Yes we'll add system tests to get end-to-end coverage, I have these stashed but got blocked by CI failures
Is the ecs version supposed to be bound to the one defined in the dependencies ? There's maybe a way to pass the pinned version to the template
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.
server/deprecation/slowlogs entries are delivered with the ecs.version already populated by ES so we don't need to do anything, for the others we could align with the dependency version. Thoughts ?
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.
Is the ecs version supposed to be bound to the one defined in the dependencies ? There's maybe a way to pass the pinned version to the template
I don't know for sure what's the ecs version defined in the dependencies is used for. As far as I know it's only used to build the docs. If it's possible to extract it from there, it could be a good option.
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/elasticsearch/data_stream/audit/elasticsearch/ingest_pipeline/default.yml
Show resolved
Hide resolved
packages/elasticsearch/data_stream/gc/_dev/test/pipeline/test-gc-logs.log-expected.json
Show resolved
Hide resolved
@@ -5,88 +5,42 @@ on_failure: | |||
field: error.message | |||
value: '{{ _ingest.on_failure_message }}' | |||
processors: | |||
- json: | |||
- rename: |
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.
Since now we're going with pure ecs formatted logs, I think we don't need to add these fields via filebeat processor. Pretty sure that at least ecs.version
isn't needed anymore, since it's already present in the log entries
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 created #4187 as a follow up
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.
Looks good! Thanks for adding these tests 💪!
We should revisit the filebeat processors present in the logs.hbs.yml
files later on. There are things there that are probably outdated or not needed anymore.
@@ -17,7 +18,7 @@ processors: | |||
if: ctx.first_char != '{' | |||
- pipeline: | |||
if: ctx.first_char == '{' | |||
name: '{< IngestPipeline "pipeline-json" >}' | |||
name: '{{ IngestPipeline "pipeline-json" }}' |
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.
Glad we caught that :)
Do you know what "methods" and "lines" aren't being covered from #4122 (comment) ? I haven't been able to find that info yet. |
Where's the info on running this locally? I read through https://github.com/elastic/integrations/blob/main/docs/testing_and_validation.md but still just get this. Not sure what I missed...
I can see the package on https://localhost:8080/search?package=elasticsearch&experimental=true though |
The fact that you can see the package but still get the error when you run the test is weird. Did you try recreating the When I first learned how to run tests locally, I followed the steps on https://github.com/elastic/elastic-package#elastic-package-test, and more specifically to pipeline tests here https://github.com/elastic/elastic-package/blob/main/docs/howto/pipeline_testing.md#running-a-pipeline-test |
We still have metrics data_streams without tests here. It's hard to tell exactly. Out of curiosity, I ran locally |
You could have a stack running with an unsupported version ? the packages only support versions |
I'd say methods should be from the lack of system tests which counts as |
Ah... I bet it's this. For some reason |
Yep, that was it!
|
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 looks good and the tests pass for me now. Would be awesome if the message made it clearer when a version mismatch was afoot but that's definitely not the goal of this PR so LGTM!
We could add the verification in the service's docker-compose and have the elasticsearch container depend on it |
I think in this case it'd be the e-p stack kibana version we need to check though right? Basically what I'm thinking is that it'd be nice if instead of just a 404 we got some information that the package exists but isn't a version match. |
Summary
Follow up from #4033
Add basic tests in elasticsearch data streams:
Because these tests verify that ingested fields are documented, I had to define additional mappings for a subset of data streams otherwise tests would fail. These fields are not required for Stack Monitoring to function but we should consider adding them to their
.monitoring-*
counterpart to keep the mappings aligned.I played with system tests but the CI job failed when creating the logs-generation container, let's investigate these in #4008. The system tests will be useful to exercise #4013