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

Update MongoPlugin to Intercept 2.x MongoDB driver classes #5429

Closed
wants to merge 4 commits into from

Conversation

asgs
Copy link

@asgs asgs commented Mar 28, 2019

This is a change related to #5392


This change is Reviewable

@CLAassistant
Copy link

CLAassistant commented Mar 28, 2019

CLA assistant check
All committers have signed the CLA.

@RoySRose
Copy link
Contributor

Hello, @asgs

All right. few things.

  1. Please develop based on master code. It's should be 1.9.0-SNAPSHOT.
  2. (Optional, but recommended) Please add IntegrationTest, You can look for class MongoDBIT_3_7_x_IT as an example
  3. Were there any interceptor code modified or added?

I'll get back to you when 1. task is taken care of :)

@codecov-io
Copy link

codecov-io commented Mar 28, 2019

Codecov Report

Merging #5429 into 1.8.x will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##            1.8.x   #5429      +/-   ##
=========================================
- Coverage   40.61%   40.6%   -0.02%     
=========================================
  Files        2653    2653              
  Lines       79811   79852      +41     
  Branches    10831   10835       +4     
=========================================
+ Hits        32416   32424       +8     
- Misses      44452   44485      +33     
  Partials     2943    2943
Impacted Files Coverage Δ
...m/navercorp/pinpoint/plugin/mongo/MongoPlugin.java 0% <0%> (ø) ⬆️
...navercorp/pinpoint/rpc/common/SocketStateCode.java 82.92% <0%> (-2.44%) ⬇️
...corp/pinpoint/rpc/stream/StreamChannelManager.java 70.68% <0%> (-1.73%) ⬇️
...rc/main/java/com/navercorp/pinpoint/test/Item.java 68.96% <0%> (ø) ⬆️
...point/rpc/client/DefaultPinpointClientHandler.java 72.61% <0%> (+0.31%) ⬆️
...orp/pinpoint/rpc/server/DefaultPinpointServer.java 79.86% <0%> (+1.04%) ⬆️
...java/com/navercorp/pinpoint/rpc/DefaultFuture.java 74.16% <0%> (+1.66%) ⬆️
...om/navercorp/pinpoint/rpc/codec/PacketDecoder.java 63.82% <0%> (+2.12%) ⬆️
...vercorp/pinpoint/rpc/packet/ClientClosePacket.java 66.66% <0%> (+11.11%) ⬆️
...p/pinpoint/rpc/client/WriteFailFutureListener.java 75% <0%> (+18.75%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f838376...df72dae. Read the comment docs.

@asgs
Copy link
Author

asgs commented Mar 28, 2019

1. Please develop based on `master` code. It's should be 1.9.0-SNAPSHOT.

@RoySRose

  1. in 1.9.0-SS, the method transform(String className, TransformCallback transformCallback) is deprecated, so the code will need to be adjusted (slightly)
  2. Also, since 1.9.0-SS is nightly (and might be unstable at the moment), we were inclined towards merging this change to the 1.8.x mainline and get the updated artifact relatively immediately

@RoySRose
Copy link
Contributor

@asgs

I think you can use whatever you have for your current operating system.
Merging into 1.8.x mainline has no meaning since there won't be any more updates with 1.8.x.(may occur if there are critical bugs, but not likely)
Codes merged in the master will only be appied in the further versions. So, normally contributions need to be merged to master branch.

About the deprecation of method transform. It won't be so hard to change if you check the codes in Mongo plugin code in master branch.

Let me know your thoughts.

@asgs
Copy link
Author

asgs commented Mar 28, 2019

@RoySRose makes sense. shall I close this and raise a new PR based off of master?

@RoySRose
Copy link
Contributor

@asgs

shall I close this and raise a new PR based off of master?

Yeah, would be great~!

@asgs asgs closed this Mar 29, 2019
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.

4 participants