-
Notifications
You must be signed in to change notification settings - Fork 782
Use Sleuth API for Redis instrumentation #2046
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
Conversation
Extracted Brave-specific Redis instrumentation to use the Sleuth API, and registered the implementation in Brave autoconfiguration. Fixes gh-2045
There was an error in my PR. I've just fixed it. |
@@ -14,27 +14,25 @@ | |||
* limitations under the License. | |||
*/ | |||
|
|||
package org.springframework.cloud.sleuth.brave.instrument.redis; | |||
package org.springframework.cloud.sleuth.instrument.redis; |
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.
That's a breaking change. I think we need to leave this one here and create a new copy in the new package.
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 for the review. How about the ClientResourcesBuilderCustomizer
interface. Is it ok that I moved it to the new package or should I duplicate that, too?
@@ -66,7 +66,9 @@ DefaultClientResources traceLettuceClientResources(ObjectProvider<ClientResource | |||
TraceLettuceClientResourcesBuilderCustomizer traceLettuceClientResourcesBuilderCustomizer(Tracing tracing, |
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.
We would need a new autoconfiguration that references the new ...cloud.sleuth.instrument.redis...
instrumentation that does not use Brave directly. The old TraceLettuceClientResourcesBuilderCustomizer
should not be registered as a bean at all (this is a breaking change but that class should not be referenced by users). In BraveRedisAutoConfiguration.java
we would need to create a bean of BraveTracing
type and in OTel we would have the OtelTracing
version.
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.
If I understood correctly, I should remove the old TraceLettuceClientResourcesBuilderCustomizer
bean registration and register two additional beans: BraveTracing
and the new TraceLettuceClientResourcesBuilderCustomizer
. Would that be correct?
Done via a08f94c |
Extracted Brave-specific Redis instrumentation to use the Sleuth API, and registered the implementation in Brave autoconfiguration.
Fixes gh-2045