-
Notifications
You must be signed in to change notification settings - Fork 791
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 support for redisson instrumentation #1132
Conversation
Hey @dengliming, this looks great! Can you implement Lines 142 to 145 in f738a50
|
Sure. thanks for quick reply : ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, just small and one big point that can probably be saved for later.
if (args instanceof CommandsData) { | ||
List<CommandData<?, ?>> commands = ((CommandsData) args).getCommands(); | ||
if (commands != null && !commands.isEmpty()) { | ||
commandName = commands.get(0).getCommand().getName() + "... [bulk]"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we do any truncation in other places, like Lettuce or DB instrumentation. It probably is a good idea but we should do it consistently for all the instrumentation, how about storing everything for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Java SDK already supports truncating too large attributes. So this better be left to them :)
} | ||
|
||
def "test batch command"() { | ||
when: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to have a more complete async test with callbacks? I notice the return type is RFuture
I wonder if our concurrent instrumentation will apply to it.
We can focus on async in another PR though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing seems to have changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
}) | ||
then: | ||
result.get(3, TimeUnit.SECONDS) | ||
assertTraces(2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is a good test! I just noticed our async redis tests don't check context propagation, we can look at that separately #1139
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dengliming!
closes #1061