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

Multideployment scenario for MP Telemetry metrics #315

Merged
merged 6 commits into from
Dec 6, 2024

Conversation

marekkopecky
Copy link
Contributor

@marekkopecky marekkopecky commented Dec 2, 2024

This PR restores multideployment scenario originally written for MP Metrics testing, removed as a part of #274 effort. This PR also rewrite such test for MP Telemetry Metrics.

PR contains 2 commits. First with original code for MP Metrics, second with changes for MP Telemetry metrics. Second commit may be more interesting.

New MP telemetry spec has been added to WF as a part of https://issues.redhat.com/browse/WFLY-19846

CI run - running - eap-8.x-microprofile-simple-face#160

Please make sure your PR meets the following requirements:

  • Pull Request contains a description of the changes
  • Pull Request does not include fixes for multiple issues/topics
  • Code is formatted, imports ordered, code compiles and tests are passing
  • Link to the passing job is provided
  • Code is self-descriptive and/or documented
  • Description of the tests scenarios is included (see Update PR template to include TPG stuff #46)

@marekkopecky marekkopecky requested a review from fabiobrz December 2, 2024 13:41
@marekkopecky
Copy link
Contributor Author

It seems that original applicationMetricsAreRegisteredAtDeploymentTime test doesn't make sense, because metrics are not available right after deployment time. Metrics are available after first call that triggers metrics, so for example:

  • first http call to servlet with injected bean with @timeout annotation (expose FT metrics)
  • first call to rest end-point with injected bean with OpenTelemetry Meter object

@fabiobrz Do you think this is expected? cc @jasondlee

@marekkopecky
Copy link
Contributor Author

Github CI test errors seems to be not related with this PR, same for internal CI pipeline run

Copy link
Member

@fabiobrz fabiobrz left a comment

Choose a reason for hiding this comment

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

Thanks @marekkopecky - I am mostly okay with the changes but I dropped some comments and also remarks about similar changes which are outstanding in other PRs too.
Feel free to let me know what you think.

@marekkopecky marekkopecky changed the title Multideployment scenario for MP Telemetry metrics Draft: Multideployment scenario for MP Telemetry metrics Dec 3, 2024
@marekkopecky marekkopecky changed the title Draft: Multideployment scenario for MP Telemetry metrics Multideployment scenario for MP Telemetry metrics Dec 4, 2024
@marekkopecky
Copy link
Contributor Author

PR rebased. Thank you for your feedback! Please note that I also added "batch-delay" property as Jason suggested, although this property actually hasn't fixed anything.

operations.writeAttribute(OPENTELEMETRY_SUBSYSTEM_ADDRESS, "batch-delay", "1");

@marekkopecky
Copy link
Contributor Author

If interested, I also started eap-8.x-microprofile-simple-face#163 internal CI run.

@fabiobrz
Copy link
Member

fabiobrz commented Dec 4, 2024

If interested, I also started eap-8.x-microprofile-simple-face#163 internal CI run.

HI @marekkopecky - It looks like the test is failing sometimes, see eap-8.x-microprofile-testsuite//jdk=openjdk-17,label_exp=RHEL8&&dynamic&&large/1143:

java.lang.NullPointerException: RESTEASY004665: uri was null
	at org.jboss.resteasy.client.jaxrs.internal.ResteasyClientImpl.target(ResteasyClientImpl.java:202)
	at org.jboss.resteasy.client.jaxrs.internal.ResteasyClientImpl.target(ResteasyClientImpl.java:30)
	at org.jboss.eap.qe.observability.containers.OpenTelemetryCollectorContainer.fetchMetrics(OpenTelemetryCollectorContainer.java:205)

could you please have a look?

@marekkopecky
Copy link
Contributor Author

Looking to job logs, it seems that docker run command failed. I'm n

12:53:28 java.lang.IllegalStateException: Starting the OTel container failed: org.jboss.eap.qe.ts.common.docker.DockerException: otel-collector-2f70bd11-1c94-4af5-877b-c830b004eeff - Starting of docker container using command: "docker run --name otel-collector-2f70bd11-1c94-4af5-877b-c830b004eeff -v /home/hudson/otel-collector/config.yaml:/etc/otel-collector/config.yaml:Z -p 4417:4317 -p 4418:4318 -p 13133:13133 -p 49152:49152 ghcr.io/open-telemetry/opentelemetry-collector-releases/opentelemetry-collector-contrib:0.103.1 --config=/etc/otel-collector/config.yaml" failed. Check that provided command is correct.
12:53:28 	at org.jboss.eap.qe.observability.containers.OpenTelemetryCollectorContainer.start(OpenTelemetryCollectorContainer.java:175)

I'm not sure how I can get more info about this error. I update start method in my last commit here, so we will see root cause of current IllegalStateException, if we see this issue later again. This said, the issue still seems to be intermittent and not related with changes in this MR, WDYT?

@fabiobrz
Copy link
Member

fabiobrz commented Dec 6, 2024

Looking to job logs, it seems that docker run command failed. I'm n

12:53:28 java.lang.IllegalStateException: Starting the OTel container failed: org.jboss.eap.qe.ts.common.docker.DockerException: otel-collector-2f70bd11-1c94-4af5-877b-c830b004eeff - Starting of docker container using command: "docker run --name otel-collector-2f70bd11-1c94-4af5-877b-c830b004eeff -v /home/hudson/otel-collector/config.yaml:/etc/otel-collector/config.yaml:Z -p 4417:4317 -p 4418:4318 -p 13133:13133 -p 49152:49152 ghcr.io/open-telemetry/opentelemetry-collector-releases/opentelemetry-collector-contrib:0.103.1 --config=/etc/otel-collector/config.yaml" failed. Check that provided command is correct.
12:53:28 	at org.jboss.eap.qe.observability.containers.OpenTelemetryCollectorContainer.start(OpenTelemetryCollectorContainer.java:175)

I'm not sure how I can get more info about this error. I update start method in my last commit here, so we will see root cause of current IllegalStateException, if we see this issue later again. This said, the issue still seems to be intermittent and not related with changes in this MR, WDYT?

Thanks @marekkopecky - Yes, the issue seems to be intermittent and for sure we can do something to improve the whole OpenTelemetryCollectorContainer API and implementation, so I've logged an issue to track this work.

With that being said, changes LGTM, thanks for this PR that reintroduces test coverage for MP related metrics.
Merging!

@fabiobrz fabiobrz self-assigned this Dec 6, 2024
@fabiobrz fabiobrz merged commit aa2a418 into jboss-eap-qe:master Dec 6, 2024
4 of 8 checks passed
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