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.

Sign up for GitHub

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

Jump to bottom

support mongo db.instance tag、db.collection tag and AggregateOperation span in mongodb-3.x-plugin and mongodb-4.x-plugin #728

Merged
merged 12 commits into from
Nov 28, 2024
Merged

support mongo db.instance tag、db.collection tag and AggregateOperation span in mongodb-3.x-plugin and mongodb-4.x-plugin #728

merged 12 commits into from
Nov 28, 2024

Conversation

youjie23
Copy link
Contributor

@youjie23 youjie23 commented Nov 22, 2024 •
edited
Loading

support mongo db.instance tag、db.collection tag and AggregateOperation span in mongodb-3.x-plugin and mongodb-4.x-plugin

@wu-sheng
Copy link
Member

Please update the test scenario about mongodb. I need to see how the pipeline data looks like in the expectation file.

@wu-sheng wu-sheng added this to the 9.4.0 milestone Nov 22, 2024
@wu-sheng
Copy link
Member

And your new changes break the plugin e2e tests. Please fix them.

@youjie23
Copy link
Contributor Author

And your new changes break the plugin e2e tests. Please fix them.

The Dead Link Checker / CheckDeadLinks ? :
[✖] https://research.google.com/pubs/pub36356.html → Status: 404

The Java-Plugin-Development-Guide.md is not in the changed files of this PR.
I'm not sure if changing it is the correct approach.
Should I ?

@wu-sheng
Copy link
Member

You don't need to worry that.

@wu-sheng
Copy link
Member

Your new testing cases seem not working well, need some fix.

@youjie23 youjie23 changed the title support mongo db.instance tag and AggregateOperation span in mongodb-3.x-plugin and mongodb-4.x-plugin support mongo db.instance tag、db.collection and AggregateOperation span in mongodb-3.x-plugin and mongodb-4.x-plugin Nov 24, 2024
wu-sheng
wu-sheng reviewed Nov 25, 2024
View reviewed changes
...lugin/src/main/java/org/apache/skywalking/apm/plugin/mongodb/v3/support/MongoSpanHelper.java Outdated
Comment on lines 74 to 89
if ("namespace".equals(field.getName())) {
namespaceField = field;
Field.setAccessible(new Field[]{field}, true);
}
if ("wrapped".equals(field.getName())) {
wrappedField = field;
Field.setAccessible(new Field[]{field}, true);
}
if ("databaseName".equals(field.getName())) {
databaseField = field;
Field.setAccessible(new Field[]{field}, true);
}
if ("collectionName".equals(field.getName())) {
collectionField = field;
Field.setAccessible(new Field[]{field}, true);
}
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain this? The more reflect we used, the more performance we impacted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review. I have made the changes using SkyWalkingDynamicField based on your feedback.

@wu-sheng
Copy link
Member

Have you verified locally? It costs a lot to run CI repeatedly.

@youjie23
Copy link
Contributor Author

Have you verified locally? It costs a lot to run CI repeatedly.

Sorry for the trouble this problem has caused.
Actually，I have run the mongodb-3.x-scenario and mongodb-4.x-scenario projects after packaging the plugins with unit tests, and checked the produced data in the UI. What bothers me is that I often make silly mistakes, such as whitespace formatting errors in JSON strings, when manually editing the expectedData.yaml files.
Is manually editing the expectedData.yaml files an incorrect approach? Are there any other convenient approaches to avoid these problems or to detect them locally? If you could provide relevant operational documentation for reference, I would be very grateful.

@wu-sheng
Copy link
Member

UT is just for testing codes you wrote, but can't verify the runtime with the real library codes.

Plugin contribution docs step <5> includes how to run this locally, https://skywalking.apache.org/docs/skywalking-java/latest/en/setup/service-agent/java-agent/java-plugin-development-guide/#contribute-plugins-to-the-apache-skywalking-repository

https://skywalking.apache.org/docs/skywalking-java/latest/en/setup/service-agent/java-agent/plugin-test/#local-test-and-pull-request-to-the-upstream

@wu-sheng
Copy link
Member

Meanwhile, you missed the PR template, which guides you to check need-to-update document. In this case, you didn't update changes.md file

@youjie23 youjie23 changed the title support mongo db.instance tag、db.collection and AggregateOperation span in mongodb-3.x-plugin and mongodb-4.x-plugin support mongo db.instance tag、db.collection tag and AggregateOperation span in mongodb-3.x-plugin and mongodb-4.x-plugin Nov 27, 2024
@youjie23
Copy link
Contributor Author

Meanwhile, you missed the PR template, which guides you to check need-to-update document. In this case, you didn't update changes.md file

Thank you for the review. Fixed that.

wu-sheng
wu-sheng approved these changes Nov 28, 2024
View reviewed changes
Copy link
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

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

LGTM.

@wu-sheng wu-sheng merged commit fc86413 into apache:main Nov 28, 2024
193 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wu-sheng wu-sheng wu-sheng approved these changes

Assignees
No one assigned
Labels
feature plugin
Projects
None yet
Milestone
9.4.0
Development

Successfully merging this pull request may close these issues.
2 participants
@youjie23 @wu-sheng