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
Improve configuration support for Observability integration #4216
Conversation
public class MongoMetricsConfigurationHelper { | ||
|
||
public static SynchronousContextProvider synchronousContextProvider(Tracer tracer, ObservationRegistry registry) { | ||
return () -> new SynchronousTraceRequestContext(tracer).withObservation(Observation.start("name", registry)); |
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 we don't have an Observation.start()
somewhere, then nothing seems to get logged in Zipkin. This is what I had used in the past inside the ZipkinIntegrationTests, but it feels quite arbitarary.
If there is a better place to "start" things, I would like to know and could adjust things accordingly.
return subscriber -> { | ||
if (subscriber instanceof CoreSubscriber<?> coreSubscriber) { | ||
return new ReactiveTraceRequestContext(coreSubscriber.currentContext()) | ||
.withObservation(Observation.start("name", registry)); |
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.
@@ -49,6 +50,10 @@ public MongoTracingObservationHandler(Tracer tracer) { | |||
this.tracer = tracer; | |||
} | |||
|
|||
public void register(ObservationRegistry observationRegistry) { |
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.
This shortcut method was purely created to slim down the registration statement users would have to add to their own code.
|
||
Map<Object, Object> map = cs.currentContext().stream() | ||
.collect(Collectors.toConcurrentMap(Entry::getKey, Entry::getValue)); | ||
if (map.containsKey(ObservationThreadLocalAccessor.KEY)) { |
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.
Why do we then store or under a different key instead of referencing it from that key later on?
*/ | ||
public class DefaultMongoHandlerObservationConvention implements MongoHandlerObservationConvention { | ||
class DefaultMongoHandlerObservationConvention implements MongoHandlerObservationConvention { |
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.
Don't we want this to be public so that the users can extend this and add more things that they want?
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 can open up the code later on if someone really wants to build on our implementation.
keyValues = keyValues.and(LowCardinalityCommandKeyNames.NET_TRANSPORT.withValue("IP.TCP"), | ||
LowCardinalityCommandKeyNames.NET_PEER_ADDR.withValue(serverAddress.getHost())); | ||
|
||
InetSocketAddress socketAddress = serverAddress.getSocketAddress(); |
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 remember but wasn't one of this calls very slow?
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.
Socket addresses must be resolved at this time already, so we use a cached instance.
|
||
@Override | ||
public String getContextualName(MongoHandlerContext context) { | ||
return context.getContextualName(); |
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.
It would be better to just define the contextual name here and not take it from the context.
*/ | ||
public class MongoHandlerContext extends Observation.Context { | ||
class MongoHandlerContext extends SenderContext<Object> { |
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 this is not public users won't be able to access it from the convention
observation.start(); | ||
|
||
requestContext.put(Observation.class, observation); | ||
requestContext.put(MongoHandlerContext.class, observationContext); |
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.
You don't need to store the context anymore cause you can retrieve it from an observation
|
||
repository.deleteAll(); | ||
|
||
System.out.println(((SimpleMeterRegistry) meterRegistry).getMetersAsString()); |
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.
You can do assertions on spans and metrics if you want to
That's merged now. |
CC @marcingrzejszczak