TST: basic grok e2e test #536
Conversation
Signed-off-by: qchea <[email protected]>
Signed-off-by: qchea <[email protected]>
Signed-off-by: qchea <[email protected]>
Signed-off-by: qchea <[email protected]>
Signed-off-by: qchea <[email protected]>
Signed-off-by: qchea <[email protected]>
Signed-off-by: qchea <[email protected]>
Signed-off-by: qchea <[email protected]>
Signed-off-by: qchea <[email protected]>
Signed-off-by: qchea <[email protected]>
Signed-off-by: qchea <[email protected]>
Signed-off-by: qchea <[email protected]>
Signed-off-by: qchea <[email protected]>
Signed-off-by: qchea <[email protected]>
| image = "opensearchproject/opensearch:${versionMap.opensearchVersion}" | ||
| } | ||
|
|
||
| task createOpenSearchDockerContainer(type: DockerCreateContainer) { |
There was a problem hiding this comment.
I am just curious. Is it possible to set up the end to end tests through creating a docker-compose.yaml. It seems like it would be a lot cleaner to initialize the tests rather than having to write all this code to set up the containers. Maybe there are some downsides to doing this that I'm unaware of?
There was a problem hiding this comment.
Thanks for calling out. There is definitely an option: https://bmuschko.com/blog/gradle-docker-compose/
Looks like from the above doc all we need to do is hardcode a docker-compose.yml. It might greatly simplify the gradle setup as you mentioned. But I think it is worth a bit more exploration to tell if it fits well into both trace-analytics and log-analytics e2e. I will open an issue for that and reserve it for a future enhancement PR on our infra.
There was a problem hiding this comment.
Great! I do think making end to end tests through docker-compose is something that would be nice down the line. It would definitely be the easiest way for external contributors to make their own end to end tests.
| /** | ||
| * This class provides the API to generate fake Apache log. | ||
| */ | ||
| public class ApacheLogFaker { |
There was a problem hiding this comment.
I like that you made your own ApacheLogFaker. It might be nice to have a single basic unit test for this log creation since changes here can cause the end to end test to fail. A failing unit test has potential to save people time trying to debug a failed end to end test.
| internet.url(), | ||
| internet.userAgentAny() |
There was a problem hiding this comment.
This doesn't need to be changed but I thought I would call it out. Technically adding this url and and userAgent isn't being used since we are matching against %{COMMONAPACHELOG} which doesn't have these. You could match these with %{COMBINEDAPACHELOG} but this is unnecessary.
| "date", "log", "clientip", "ident", "auth", "timestamp", "verb", "request", "httpversion", "response", "bytes"); | ||
| retrievedDocs.forEach(expectedDoc -> { | ||
| for (String key: expectedDocKeys) { | ||
| Assert.assertNotNull(expectedDoc.get(key)); |
There was a problem hiding this comment.
Is there a way to do a check for the values themselves instead of just checking for non-null? Since ordering in sending and retrieving isn't guaranteed I can see this being slightly annoying.
There was a problem hiding this comment.
I'd recommend:
assertThat(expectedDoc, hasKey(key));
assertThat(expectedDoc.get(key), equalTo(PLACEHOLDER));
Using hasKey is much clearer in conveying intent.
There was a problem hiding this comment.
@dlvenable @graytaylor0 Thanks for the comments.
I think hasKey makes a lot of sense. For checking values themselves, it seems adding little value to me b/c essentially it is testing the business logic of grok-prepper(java-grok) which should be covered by the unit test itself, here the main intention is to make sure the logs has been grokked before exporting to opensearch.
| return HttpData.ofUtf8(jsonData); | ||
| } | ||
|
|
||
| public static void main(String[] args) throws JsonProcessingException { |
There was a problem hiding this comment.
I'm not sure I understand why we need this main method here with the print statement?
There was a problem hiding this comment.
Ah. That is my mis. Great catch! Will remove that.
e2e-test/log/build.gradle
Outdated
|
|
||
|
|
||
| import com.bmuschko.gradle.docker.DockerRemoteApiPlugin | ||
| import com.bmuschko.gradle.docker.tasks.container.* |
There was a problem hiding this comment.
can we get rid of these wildcard imports?
e2e-test/log/build.gradle
Outdated
| * End to end test. Spins up OpenSearch and DataPrepper docker containers, then runs the integ test | ||
| * Stops the docker containers when finished | ||
| */ | ||
| task basicGrokEndToEndTest(type: Test) { |
There was a problem hiding this comment.
Rather than basicGrokEndToEndTest, I'd prefer basicLogIngestEndToEndTest. It is somewhat parallel to "trace analytics" which we use in other tests. And it is consistent with the project e2e-test/log.
There was a problem hiding this comment.
Also, it tests more than just grok since it simulates the whole "basic" log ingest pipeline.
Signed-off-by: qchea <[email protected]>
Signed-off-by: qchea <[email protected]>
Signed-off-by: qchea <[email protected]>
Signed-off-by: qchea <[email protected]>
Description
This PR
Issues Resolved
[List any issues this PR will resolve]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.