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

[WFLY-18943] Fix automated testing of OpenTelemetry QS under OpenShift #870

Closed
wants to merge 1 commit into from

Conversation

jasondlee
Copy link
Contributor

https://issues.redhat.com/browse/WFLY-18943

Build with openshift profile
Clean up YAML
Modify how WF is configured under OS build
Fix application deployment in POM file

@jasondlee jasondlee force-pushed the WFLY-18943 branch 3 times, most recently from 353d6f5 to 5a5f6df Compare January 29, 2024 23:41
Build with openshift profile
Clean up YAML
Modify how WF is configured under OS build
Add instructions for deploying otel collector on OS
Copy link
Contributor

@emmartins emmartins left a comment

Choose a reason for hiding this comment

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

Some changes are not needed, you may find the proposed changes at #872

@@ -2,5 +2,11 @@ build:
uri: https://github.com/wildfly/quickstart.git
ref: main
contextDir: opentelemetry-tracing
env:
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed, the openshift profile is used by default.

@@ -0,0 +1,16 @@
=== Install OpenTelemetry Collector on OpenShift

The functionality of this quickstart depends on a running instance of the https://opentelemetry.io/docs/collector/[OpenTelemetry Collector]. To deploy and configure the OpenTelemetry Collector, you will need to apply a set of configurations to your OpenShift cluster. In your terminal, run the following command to configure the OpenTelemetry Collector as well as any external routes needed:
Copy link
Contributor

@emmartins emmartins Jan 30, 2024

Choose a reason for hiding this comment

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

IMHO we should not ask them to run the huge command first, here is my suggestion:

The functionality of this quickstart depends on a running instance of the https://opentelemetry.io/docs/collector/[OpenTelemetry Collector].

To deploy and configure the OpenTelemetry Collector, you will need to apply a set of configurations to your OpenShift cluster, to configure the OpenTelemetry Collector as well as any external routes needed:

[source,options="nowrap",subs="+attributes"]
----
include::charts/opentelemetry-collector.yaml[]
----

To make things simpler, you can find these commands in `charts/opentelemetry-collector.yaml`, and to apply them run the following command in your terminal:

[source]
----
$ oc apply -f charts/opentelemetry-collector.yaml
----

<scripts>
<script>${basedir}/configure-opentelemetry.cli</script>
</scripts>
<commands>
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO it is preferable to use the cli script, since that is used also for baremetal config. I did some tests with Openshift Local and a modified branch of and the following worked fine:

                            <packaging-scripts>
                                <packaging-script>
                                    <scripts>
                                        <script>${basedir}/configure-opentelemetry.cli</script>
                                    </scripts>
                                    <resolve-expressions>false</resolve-expressions>
                                </packaging-script>
                            </packaging-scripts>

@@ -220,6 +230,9 @@
<layer>jaxrs-server</layer>
<layer>opentelemetry</layer>
</layers>
<plugin-options>
Copy link
Contributor

Choose a reason for hiding this comment

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

dupe, please remove this change

@jasondlee jasondlee closed this Jan 30, 2024
@jasondlee
Copy link
Contributor Author

jasondlee commented Jan 30, 2024

This PR has been superseded by #872 .

@jasondlee jasondlee deleted the WFLY-18943 branch January 30, 2024 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants