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

Enhance MongoDb otel integration #40714

Merged
merged 1 commit into from
Jun 6, 2024
Merged

Conversation

vkn
Copy link
Contributor

@vkn vkn commented May 18, 2024

This PR continues #40191

  • Fix parent-child spans for reactive request
  • Add docs
  • Add more integration and unit tests

cc @brunobat

This comment has been minimized.

Copy link

github-actions bot commented May 18, 2024

🙈 The PR is closed and the preview is expired.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

@brunobat brunobat left a comment

Choose a reason for hiding this comment

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

A few things:

  1. You need to add a module to quarkus-integration-tests-parent:
  <module>opentelemetry-mongodb-client-instrumentation</module>

Bellow <module>opentelemetry-jdbc-instrumentation</module>

  1. These tests should only run when test container tests are active, to save build time. Tests need to be disabled by default:
<plugin>
    <artifactId>maven-surefire-plugin</artifactId>
    <configuration>
        <skip>true</skip>
    </configuration>
</plugin>
<plugin>
    <artifactId>maven-failsafe-plugin</artifactId>
    <configuration>
        <skip>true</skip>
    </configuration>
</plugin>

And a new profile must be added. There is a task on the CI to run only these test-containers tests:

 <profiles>
    <!-- Note: the container is started via Dev Services -->
    <profile>
        <id>test-mongodb</id>
        <activation>
            <property>
                <name>test-containers</name>
            </property>
        </activation>
        <build>
            <plugins>
                <plugin>
                    <artifactId>maven-surefire-plugin</artifactId>
                    <configuration>
                        <skip>false</skip>
                    </configuration>
                </plugin>
                <plugin>
                    <artifactId>maven-failsafe-plugin</artifactId>
                    <configuration>
                        <skip>false</skip>
                    </configuration>
                </plugin>
            </plugins>
        </build>
    </profile>
</profiles>

This comment has been minimized.

This comment has been minimized.

@brunobat
Copy link
Contributor

brunobat commented May 21, 2024

Can you also please add the it project opentelemetry-mongodb-client-instrumentation to this line:

"test-modules": "picocli-native, gradle, micrometer-mp-metrics, micrometer-prometheus, logging-json, jaxp, jaxb, opentelemetry, opentelemetry-jdbc-instrumentation, opentelemetry-redis-instrumentation, web-dependency-locator",

Otherwise the tests will not run on native.

@quarkus-bot quarkus-bot bot added the area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure label May 21, 2024
@vkn
Copy link
Contributor Author

vkn commented May 21, 2024

Can you also please add the it project opentelemetry-mongodb-client-instrumentation to this line:

done

This comment has been minimized.

@brunobat
Copy link
Contributor

brunobat commented May 21, 2024

@vkn, I see these exceptions on the build, do you have an Idea of the cause?
https://github.com/quarkusio/quarkus/actions/runs/9173102213/job/25221800658?pr=40714#step:16:2124

[mongod output] 2024-05-21 11:42:16,111 ERROR [io.qua.ver.htt.run.QuarkusErrorHandler] (vert.x-eventloop-thread-3) HTTP Request to /reactive-books/invalid failed, error id: 4980e026-f2c7-4790-b8eb-eab7111de4e7-1: com.mongodb.MongoQueryException: Command failed with error 2 (BadValue): 'unknown top level operator: $invalidop. If you have a field name that starts with a '$' symbol, consider using $getField or $setField.' on server localhost:27017. The full response is {"ok": 0.0, "errmsg": "unknown top level operator: $invalidop. If you have a field name that starts with a '$' symbol, consider using $getField or $setField.", "code": 2, "codeName": "BadValue"}

@vkn
Copy link
Contributor Author

vkn commented May 21, 2024

@vkn, I see these exceptions on the build, do you have an Idea of the cause? https://github.com/quarkusio/quarkus/actions/runs/9173102213/job/25221800658?pr=40714#step:16:2124

[mongod output] 2024-05-21 11:42:16,111 ERROR [io.qua.ver.htt.run.QuarkusErrorHandler] (vert.x-eventloop-thread-3) HTTP Request to /reactive-books/invalid failed, error id: 4980e026-f2c7-4790-b8eb-eab7111de4e7-1: com.mongodb.MongoQueryException: Command failed with error 2 (BadValue): 'unknown top level operator: $invalidop. If you have a field name that starts with a '$' symbol, consider using $getField or $setField.' on server localhost:27017. The full response is {"ok": 0.0, "errmsg": "unknown top level operator: $invalidop. If you have a field name that starts with a '$' symbol, consider using $getField or $setField.", "code": 2, "codeName": "BadValue"}

Yes. This is the test for failed query case. So the exception log is expected

Copy link
Contributor

@brunobat brunobat left a comment

Choose a reason for hiding this comment

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

Thanks for the effort @vkn!

This comment has been minimized.

This comment has been minimized.

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Nice addition. I added a few suggestions for the doc.

docs/src/main/asciidoc/mongodb.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/mongodb.adoc Show resolved Hide resolved
@gsmet
Copy link
Member

gsmet commented May 28, 2024

@vkn nice work! I asked a few micro changes for the doc. Also, it would be awesome if you could squash the commits in one when done (we use merge commits and we can't squash with the GitHub UI when merging).
Let me know if you need help, I can help if needed.

Continue PR quarkusio#40191

- Add docs
- Add tests
- Fix parent-child spans for reactive request
@vkn vkn force-pushed the mongodb-otel-enhancements branch from 24ea28a to c9480af Compare May 28, 2024 10:42
@vkn
Copy link
Contributor Author

vkn commented May 28, 2024

@gsmet I've applied your suggestions and squashed the commits

Copy link

quarkus-bot bot commented May 28, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit c9480af.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

⚠️ There are other workflow runs running, you probably need to wait for their status before merging.

Copy link

quarkus-bot bot commented May 28, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit c9480af.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.


Flaky tests - Develocity

⚙️ JVM Tests - JDK 21

📦 extensions/smallrye-reactive-messaging-kafka/deployment

io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase.sseStream - History

  • Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase Expecting size of: [] to be greater than or equal to 2 but was 0 within 10 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: 
Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase 
Expecting size of:
  []
to be greater than or equal to 2 but was 0 within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)

📦 integration-tests/reactive-messaging-kafka

io.quarkus.it.kafka.KafkaConnectorTest.testFruits - History

  • Assertion condition defined as a Lambda expression in io.quarkus.it.kafka.KafkaConnectorTest expected: <6> but was: <5> within 10 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: Assertion condition defined as a Lambda expression in io.quarkus.it.kafka.KafkaConnectorTest expected: <6> but was: <5> within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.untilAsserted(ConditionFactory.java:790)
	at io.quarkus.it.kafka.KafkaConnectorTest.testFruits(KafkaConnectorTest.java:63)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)

@vkn
Copy link
Contributor Author

vkn commented Jun 6, 2024

Hi @gsmet, is there anything else needed from me before merging?

@gsmet gsmet merged commit 8416855 into quarkusio:main Jun 6, 2024
54 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.12 - main milestone Jun 6, 2024
@gsmet
Copy link
Member

gsmet commented Jun 6, 2024

Nope, just me getting to the second page of PRs :).

Thanks for your efforts!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

3 participants