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.

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 docker test #1827

Merged
merged 8 commits into from
Feb 3, 2025
Merged

Elasticsearch docker test #1827

merged 8 commits into from
Feb 3, 2025

Conversation

kinjalh
Copy link
Member

@kinjalh kinjalh commented Jan 30, 2025

Add an unit test that uses a local elasticsearch index to check if it is able to retrieve logs from.
This requires spinning up an elasticsearch container, creating an index on it, adding log entries to the index, querying the index for the logs, and then deleting the index.

Test Plan

mix test test/console/graphql/queries/observability_queries_test.exs

Checklist

  • If required, I have updated the Plural documentation accordingly.
  • I have added tests to cover my changes.
  • I have added a meaningful title and summary to convey the impact of this PR to a user.

@@ -92,6 +92,13 @@ defmodule Console.Logs.Provider.Elastic do
defp add_facets(q, _), do: q

defp facets(resp) do
# make sure that kubernetes.node has at least an empty map
Copy link
Member

Choose a reason for hiding this comment

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

this todo might be able to be removed


# We want a fresh index so delete the index if it already exists
# Just in case test fails we want to make sure next run doesn't reuse old index
with true <- ES.index_exists?(@elastic_cluster_url, @elastic_index_name) do
Copy link
Member

Choose a reason for hiding this comment

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

the delete and recreate should be moved to a mix task and added to the mix test alias in mix.exs

ES.create_index(@elastic_cluster_url, @elastic_index_name)

ES.index_documents(@elastic_cluster_url, @elastic_index_name, [
%{
Copy link
Member

Choose a reason for hiding this comment

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

let's move these documents to helper function in elastic utils, eg something like:

def log_document(%Service{}, line) do
  ...map data for log
end

And that way you can just index by doing something like:

log_document(svc, "valid log") |> index()
log_document(svc, "valid log2") |> index()
log_document(svc2, "valid log") |> index()
refresh()

Copy link
Member

@michaeljguarino michaeljguarino Jan 31, 2025

Choose a reason for hiding this comment

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

you should also just import the elastic helper so it's quicker to call the functions I think but that's more aesthetics

@kinjalh kinjalh force-pushed the elasticsearch-docker-test branch from e06827d to 82c1fcb Compare February 3, 2025 14:41
@kinjalh kinjalh added the enhancement New feature or request label Feb 3, 2025
@@ -8,6 +8,16 @@ services:
POSTGRES_PASSWORD: postgres
volumes:
- database_data:/var/lib/postgresql/data
es:
Copy link
Member

Choose a reason for hiding this comment

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

i think this might need a volume to really behave nicely

@@ -0,0 +1,32 @@
# lib/mix/tasks/elasticsearch_setup.ex
Copy link
Member

Choose a reason for hiding this comment

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

rm

# the index gets set up using a mix task before the test is run, so we can index directly
log_document(svc, "valid log message") |> index_doc(@elastic_cluster_url, @elastic_index_name)
log_document(svc, "another valid log message") |> index_doc(@elastic_cluster_url, @elastic_index_name)
refresh(@elastic_cluster_url, @elastic_index_name)
Copy link
Member

Choose a reason for hiding this comment

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

I'd move the cluster url into your elasticsearch helper module as a default arg (so users don't need to specify it w/ every call to index_doc or refresh

Copy link
Member

Choose a reason for hiding this comment

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

use default args in refresh here too

# the index gets set up using a mix task before the test is run, so we can index directly
log_document(svc, "valid log message") |> index_doc(@elastic_cluster_url, @elastic_index_name)
log_document(svc, "another valid log message") |> index_doc(@elastic_cluster_url, @elastic_index_name)
refresh(@elastic_cluster_url, @elastic_index_name)
Copy link
Member

Choose a reason for hiding this comment

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

use default args in refresh here too

@kinjalh kinjalh merged commit ab99928 into master Feb 3, 2025
7 checks passed
@kinjalh kinjalh deleted the elasticsearch-docker-test branch February 3, 2025 18:12
kinjalh added a commit that referenced this pull request Feb 3, 2025
kinjalh added a commit that referenced this pull request Feb 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants