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

Add mongo commands to otel span attributes #40191

Merged
merged 2 commits into from
Apr 29, 2024

Conversation

vkn
Copy link
Contributor

@vkn vkn commented Apr 22, 2024

Solving #28473

A simple approach to add mongo commands to traces using CommandListener. Similar approach as it was in quarkus-opentracing but without relying on https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/main/instrumentation/mongo/mongo-3.1/library, but using the similiar though simpler code.

Copy link

quarkus-bot bot commented Apr 22, 2024

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

  • title should preferably start with an uppercase character (if it makes sense!)

This message is automatically generated by a bot.

@vkn vkn changed the title [WIP] Add mongo commands to otel span attributes Add mongo commands to otel span attributes Apr 23, 2024
@vkn vkn marked this pull request as ready for review April 23, 2024 11:07
@gsmet gsmet force-pushed the mongodb-client-opentelemetry branch from 9e109de to 8e551cf Compare April 26, 2024 15:22
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.

This looks like a very good contribution. I took the liberty to squash the commits and rebase.

@brunobat @loicmathieu could you have a look? It looks like a very nice addition.

Copy link
Contributor

@loicmathieu loicmathieu left a comment

Choose a reason for hiding this comment

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

Overall it's good to me, very small integration so it's better not to rely on an external lib for that.

I add one suggestion but we can merge as is.

I'm also concern about the cost of adding one integration test for that, it's a lot for such small integration, I wonder if it's really needed WDYT @gsmet ? We can either add that in the existing MongoDB test or only rely on the unit test.

This comment has been minimized.

@loicmathieu
Copy link
Contributor

Will also fix #40191

Copy link

quarkus-bot bot commented Apr 29, 2024

Status for workflow Quarkus CI

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

✅ 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.

@loicmathieu loicmathieu merged commit ca27e0c into quarkusio:main Apr 29, 2024
20 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.11 - main milestone Apr 29, 2024
@FroMage
Copy link
Member

FroMage commented May 3, 2024

This is interesting. I wonder if we can add the same telemetry support to ORM and HR/Panache static methods.

@brunobat
Copy link
Contributor

brunobat commented May 3, 2024

Was on PTO and only now I saw this...
The implementation should have been made in the OTel extension and not in the Mongo db one... MongoDB should not depend on OTel.
All other instrumentations work like that.

@vkn are you available to refactor the code in that way?

Also we lack documentation entries and additional IT tests for Uni, Multi, parent-child relationships and failure cases.

Also, the assertions need to verify the attributes of the spans.

@vkn
Copy link
Contributor Author

vkn commented May 3, 2024

@vkn are you available to refactor the code in that way?

@brunobat yes

Context parentContext = Context.current();
if (instrumenter.shouldStart(parentContext, event)) {
Context context = instrumenter.start(parentContext, event);
requestMap.put(event.getRequestId(), new ContextEvent(context, event));
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm concerned with the amount of data you are storing here... can you give an example of one of this events?
Can you just store what you need later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brunobat it is not big, just description of mongo connection, db name and command. You can inspect it by using BookResourceTest. But I could create another record to just save command name and the command itself, if you prefer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brunobat I've moved the implementation to opentelemetry extension. Is this enough for a new PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is but there are also other improvements needed:

Also we lack documentation entries and additional IT tests for Uni, Multi, parent-child relationships and failure cases.

Also, the assertions need to verify the attributes of the spans.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brunobat Opened #40472

I'm not sure about additional IT tests. There is already a test for for reactive endpoint with assertion for attributes:

    @Test
    void reactiveClient() {
        testInsertBooks("/reactive-books");
        assertTraceAvailable("my-reactive-collection");
    }

@vkn vkn mentioned this pull request May 6, 2024
vkn added a commit to vkn/quarkus that referenced this pull request May 17, 2024
Continue PR quarkusio#40191

- Add docs
- Add tests
- Fix parent-child spans for reactive request
vkn added a commit to vkn/quarkus that referenced this pull request May 18, 2024
Continue PR quarkusio#40191

- Add docs
- Add tests
- Fix parent-child spans for reactive request
vkn added a commit to vkn/quarkus that referenced this pull request May 28, 2024
Continue PR quarkusio#40191

- Add docs
- Add tests
- Fix parent-child spans for reactive request
holly-cummins pushed a commit to holly-cummins/quarkus that referenced this pull request Jul 31, 2024
Continue PR quarkusio#40191

- Add docs
- Add tests
- Fix parent-child spans for reactive request
danielsoro pushed a commit to danielsoro/quarkus that referenced this pull request Sep 20, 2024
Continue PR quarkusio#40191

- Add docs
- Add tests
- Fix parent-child spans for reactive request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants