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

Lettuce instrumentation - optimization to avoid extra toString() #8984

Merged
merged 1 commit into from
Jul 20, 2023

Conversation

breedx-splk
Copy link
Contributor

Relates to #8907

The instrumentation was calling SingularArgument.toString() for every argument. In the case of KeyArgument and ValueArgument, the implementations of toString() serve only to wrap the value in a string literal key<...> or value<...> (respectively), which we then parsed back out.

So this change doesn't call toString() in those two cases, instead jumping right to the contents (essentially inlining the important part of the toString() implementation from the lettuce source). This both bypasses the extra parsing/unwrapping but also eliminates a String.format() that generated it in the first place.

All other argument types still use the vanilla toString() approach.

Comment on lines +42 to +43
ValueArgument valueArg = (ValueArgument) argument;
return stringCodec.decodeValue(valueArg.codec.encodeValue(valueArg.val));
Copy link
Contributor Author

@breedx-splk breedx-splk Jul 19, 2023

Choose a reason for hiding this comment

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

Taken from the existing source of ValueArgument.toString(). One note: the codec is being reused here to avoid allocations.

Copy link
Contributor

Choose a reason for hiding this comment

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

I initially implemented it similarly to what you have done but chose to use substring because of simplicity and reduced dependency on lettuce internals.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, agreed that this feels a tad more fragile. Hopefully it's a fair trade off for performance tho.

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

👍

@trask trask merged commit 600c587 into open-telemetry:main Jul 20, 2023
44 checks passed
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