-
Notifications
You must be signed in to change notification settings - Fork 3k
Implement Kibana/OpenSearch Dashboards dev services #51164
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
base: main
Are you sure you want to change the base?
Conversation
|
/cc @gsmet (elasticsearch), @loicmathieu (elasticsearch), @marko-bekhta (elasticsearch) |
marko-bekhta
left a comment
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.
Hey 👋🏻
thanks for the PR! 🙂
Could you please use rebase instead of adding merge commits? Something like:
git checkout main
git fetch upstream
git rebase upstream/main
where upstream points to this repo (git remote add upstream https://github.com/quarkusio/quarkus.git that is if you haven't set up the upstream yet) usually should do it for your current branch (main from which you've opened this PR) 🙂
I think it would be better if you just create another processor for the dashboards (i.e. DevServicesElasticsearchDashboardsProcessor), that way you'd easiely spot if you are not missing any stop/restart actions + the processor for the Elasticsearch services is about to change to work with multiple services at the same time.
You've made good progress here already 👍🏻
Have you had a chance to look into this part about dev ui as well:
it would be a great start if we provided either an option to launch Kibana in the Elasticsearch dev services (and include a link to it in the Dev UI),
it can go as a follow up if not 🙂
...io/quarkus/elasticsearch/restclient/common/deployment/DevServicesElasticsearchProcessor.java
Outdated
Show resolved
Hide resolved
...io/quarkus/elasticsearch/restclient/common/deployment/DevServicesElasticsearchProcessor.java
Outdated
Show resolved
Hide resolved
...io/quarkus/elasticsearch/restclient/common/deployment/DevServicesElasticsearchProcessor.java
Outdated
Show resolved
Hide resolved
...io/quarkus/elasticsearch/restclient/common/deployment/DevServicesElasticsearchProcessor.java
Outdated
Show resolved
Hide resolved
...io/quarkus/elasticsearch/restclient/common/deployment/DevServicesElasticsearchProcessor.java
Outdated
Show resolved
Hide resolved
...io/quarkus/elasticsearch/restclient/common/deployment/DevServicesElasticsearchProcessor.java
Outdated
Show resolved
Hide resolved
...io/quarkus/elasticsearch/restclient/common/deployment/DevServicesElasticsearchProcessor.java
Outdated
Show resolved
Hide resolved
...csearch-rest-client-common/deployment/src/main/resources/elasticsearch-devservice.properties
Outdated
Show resolved
Hide resolved
|
Thanks for all your comments. So I would move all my changes into a separate class DevServicesDashboardProcessor with its own properties files and address your proposed changes. Regarding the Dev Ui. Yes I had that in mind but I planned it as a follow up. |
gsmet
left a comment
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.
Thanks for this work, I see we are making good progress. I added a small question.
...o/quarkus/elasticsearch/restclient/common/deployment/ElasticsearchCommonBuildTimeConfig.java
Outdated
Show resolved
Hide resolved
|
I am in the refactoring to move everything related to dashboards into a new processor class named |
oh yes, you could make the ES services produce a build item(s) for you (it would contain a list of hosts) and then you'd use that build items as an input to your dashboards step: ...
startElasticsearchDevService(
...
BuildProducer<ElasticsearchHostBuildItem> elasticsearchHosts
.. .) {
...
elasticsearchHosts.produce(...)
}and then in the dashboards: startElasticsearchDashboardsDevService(
...
List<ElasticsearchHostBuildItem> elasticsearchHosts
.. .) {
...
List<String> eshosts = getAllEsHosts(elasticsearchHosts);
..
}(maybe you could come up with a better name for |
…n/java/io/quarkus/elasticsearch/restclient/common/deployment/DevServicesElasticsearchProcessor.java Fixed copy paste error when shutting down dashboard dev service as proposed by Marko Co-authored-by: Marko Bekhta <[email protected]>
…n/java/io/quarkus/elasticsearch/restclient/common/deployment/DevServicesElasticsearchProcessor.java Fixed copy paste error as proposed by Marko. Co-authored-by: Marko Bekhta <[email protected]>
Fixed duplicate Kibana image prop as proposed by Marko. Co-authored-by: Marko Bekhta <[email protected]>
…n/java/io/quarkus/elasticsearch/restclient/common/deployment/DevServicesElasticsearchProcessor.java Fixed copy paste error as proposed by Marko. Co-authored-by: Marko Bekhta <[email protected]>
…vice to dashboards devservice
|
All points addressed so far. Please review again. |
This comment has been minimized.
This comment has been minimized.
|
🎊 PR Preview f8ca654 has been successfully built and deployed to https://quarkus-pr-main-51164-preview.surge.sh/version/main/guides/
|
This comment has been minimized.
This comment has been minimized.
|
If anything else is needed, please let me know. |
marko-bekhta
left a comment
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.
hey 👋🏻 🙂
sorry for not getting back to you sooner. Thanks for splitting out the Kibana processor!
I've added a couple comments where we could cleanup a few things.
.../elasticsearch/restclient/common/deployment/DevServicesElasticsearchDashboardsProcessor.java
Outdated
Show resolved
Hide resolved
.../elasticsearch/restclient/common/deployment/DevServicesElasticsearchDashboardsProcessor.java
Outdated
Show resolved
Hide resolved
.../elasticsearch/restclient/common/deployment/DevServicesElasticsearchDashboardsProcessor.java
Outdated
Show resolved
Hide resolved
.../elasticsearch/restclient/common/deployment/DevServicesElasticsearchDashboardsProcessor.java
Outdated
Show resolved
Hide resolved
.../elasticsearch/restclient/common/deployment/DevServicesElasticsearchDashboardsProcessor.java
Show resolved
Hide resolved
| .map(host -> "http://" + host.replace("localhost", "host.docker.internal")) | ||
| .collect(Collectors.toSet()); | ||
| } else { | ||
| Optional<ContainerAddress> maybeContainerAddressSearchBackend = Optional.empty(); |
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.
| Optional<ContainerAddress> maybeContainerAddressSearchBackend = Optional.empty(); |
seems to be unused ...
| DockerImageName resolvedImageName = resolveDashboardImageName(config, resolvedDistribution); | ||
|
|
||
| Set<String> opensearchHosts; | ||
| if (buildItemConfig.hostsConfigProperties.stream().anyMatch(ConfigUtils::isPropertyNonEmpty)) { |
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.
Thinking more about this ... hosts are a runtime property:
Line 22 in 15c3f56
| List<InetSocketAddress> hosts(); |
so we won't necessarily get them here...
Do you happen to know if there's an easy way we can "append" the list of hosts we pass to kibana/dashboards once they've starteed and are running ? (maybe some REST API ?)
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.
Thinking more about it ...
maybe let's start with using just List<DevservicesElasticsearchConnectionBuildItem> elasticsearchConnectionBuildItems for now. And find a way to add the non-devservices instances later.
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.
If quarkus.elasticsearch.hosts is set, the Elasticsearch dev service is not started, and we don't have anyDevservicesElasticsearchConnectionBuildItems. In this case the Dashboard processor reads the quarkus.elasticsearch.hosts.
| log.info( | ||
| "no elasticsearch hosts config property found, using the host of theelasticsearch dev services container to connect"); |
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.
| log.info( | |
| "no elasticsearch hosts config property found, using the host of theelasticsearch dev services container to connect"); | |
| log.debug( | |
| "No Elasticsearch hosts config property found, using the host of the Elasticsearch dev services container to connect"); |
|
|
||
| private CreatedContainer createKibanaContainer(ElasticsearchDevServicesBuildTimeConfig config, | ||
| DockerImageName resolvedImageName, String defaultNetworkId, boolean useSharedNetwork, | ||
| LaunchModeBuildItem launchMode, DevServicesComposeProjectBuildItem composeProjectBuildItem, |
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.
| LaunchModeBuildItem launchMode, DevServicesComposeProjectBuildItem composeProjectBuildItem, |
I think we only needed these to lookup the container, but you've done that already before calling these create methods, so maybe let's remove the paramteres here ?
| : DEV_SERVICE_OPENSEARCH))); | ||
| } | ||
|
|
||
| private Distribution resolveDistribution(ElasticsearchDevServicesBuildTimeConfig config, |
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.
If we go with the idea of using just the DevservicesElasticsearchConnectionBuildItem and having the distribution in there, we probably can get most of these methods back into this processor and keep them private.
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.
Not this one because if quarkus.elasticsearch.hosts is set, the Elasticsearch dev service is not started, and we don't have anyDevservicesElasticsearchConnectionBuildItems. In this case the Dashboard processor reads the quarkus.elasticsearch.hosts and can only get the distribution from quarkus.elasticsearch.devservices.distribution.
…n/java/io/quarkus/elasticsearch/restclient/common/deployment/DevServicesElasticsearchDashboardsProcessor.java Co-authored-by: Marko Bekhta <[email protected]>
…n/java/io/quarkus/elasticsearch/restclient/common/deployment/DevServicesElasticsearchDashboardsProcessor.java Co-authored-by: Marko Bekhta <[email protected]>
…n/java/io/quarkus/elasticsearch/restclient/common/deployment/DevServicesElasticsearchDashboardsProcessor.java Co-authored-by: Marko Bekhta <[email protected]>
…n/java/io/quarkus/elasticsearch/restclient/common/deployment/DevServicesElasticsearchDashboardsProcessor.java Co-authored-by: Marko Bekhta <[email protected]>
…n/java/io/quarkus/elasticsearch/restclient/common/deployment/DevServicesElasticsearchDashboardsProcessor.java Co-authored-by: Marko Bekhta <[email protected]>
…on from elasticsearchConnectionBuildItems.

This change will add dashboard services to the dev services for extensions/elasticsearch-rest-client and extensions/elasticsearch-java-client. If the distribution is elasticsearch, it will start Kibana as the dashboard. If the distribution is opensearch, it will start OpenSearch-dashboards as the dashboard.