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

Jedis 4.0.0 #698

Merged
merged 5 commits into from
Mar 3, 2022
Merged

Jedis 4.0.0 #698

merged 5 commits into from
Mar 3, 2022

Conversation

tbradellis
Copy link
Contributor

This adds instrumentation for Jedis 4.0.0+)
Maintains the same instrumentation points as previous versions. JedisPubSub and Connection instrumentation.

Related Github Issue

#641
https://github.com/redis/jedis/tree/4.0

Testing

Uses verifyInstrumentation in the usual way.
Additional integration test with local apps to ensure correct counts and no loss of instrumentation features.

Checks

[ NA] Are your contributions backwards compatible with relevant frameworks and APIs?
[ ] Does your code contain any breaking changes? Please describe.
[ ] Does your code introduce any new dependencies? Please describe.

@tbradellis tbradellis marked this pull request as ready for review February 15, 2022 16:09
@tbradellis
Copy link
Contributor Author

tbradellis commented Feb 15, 2022

This is ready for review. I have one question concerning a new method. In this new method there is a ProtocolCommand and a Keyword
https://github.com/redis/jedis/blob/ea18faa55728de4697ecf7f504aa826491a752d6/src/main/java/redis/clients/jedis/Connection.java#L139

Example:
https://github.com/redis/jedis/blob/ea18faa55728de4697ecf7f504aa826491a752d6/src/main/java/redis/clients/jedis/Jedis.java#L3377
This code already reports the command given. I suppose we could include the keyword as an attribute, but I don't know if it is necessary. I'm currently not tracing this method, but if we aren't going to report the Keyword, then tracing is not needed because it is handled in sendCommand(CommandArguments args)
What do you think? (previous instrumentation module versions do not report keyword)

instrumentation/jedis-4.0.0/build.gradle Outdated Show resolved Hide resolved
Comment on lines 31 to 38
@NewField
private String host;

@NewField
private int port;

@NewField
private HostAndPort hostAndPort;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you do like the old instrumentation and call getHostAndPort() when you need the info.
I worry that this will add an increase in memory due to the new classes that will be created for these fields and the maps they require.

The call to getHostAndPort is pretty straightforward and shouldn't add much overhead. Or it should be comparable with the map lookup that is needed with the (a)NewField.

And as an added benefit, you instrument one less method.

}

private void reportMethodAsExternal(ProtocolCommand command, String serverUsed, int serverPortUsed) {
String operation = "unknown";
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, the call to getHostAndPort could be done inside this method.

@NewField
private int port;

@Trace
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to trace this new method?
It may cause confusion when people upgrade Jedis and see a new span in their traces.

@tbradellis
Copy link
Contributor Author

@aonuki I noticed that. maybe there is an opportunity in this JedisPubSub_Instrumentation class to also drop some @newfield usage and save a little memory (like in the Connection_Instrumentation).

The only thing I see that might make a difference is the number of calls I’d need to make to get the host and port in each of the methods. Do you think this would be a good change here or would leaving it in this case make sense for any reason? Readability or anything else?

@meiao
Copy link
Contributor

meiao commented Mar 2, 2022

In JedisPubSub, you could move the call to inside the reportMethodAsExternal method and remove the host and port parameters.

@tbradellis
Copy link
Contributor Author

Oh, derp...yes that could be done the same too. Thanks!!

@tbradellis tbradellis merged commit d7e2c27 into main Mar 3, 2022
@tbradellis tbradellis deleted the jedis-4.0.0 branch March 3, 2022 01:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants