Ensure that the name generated in opentelemetry matches the endpoint being called #44523
Conversation
…dpoint being called
This comment was marked as resolved.
This comment was marked as resolved.
|
/cc @brunobat (opentelemetry), @radcortez (opentelemetry) |
geoand
changed the title
|
Thanks a lot for this! I agree the problem needs to be fixed, but I am not convinced this is the proper solution. I will spend some time looking into it as I think the way |
| private HandlerChain(ClientRestHandler clientCaptureCurrentContextRestHandler, | ||
| ClientRestHandler clientSwitchToRequestContextRestHandler, ClientRestHandler clientSendHandler, | ||
| ClientRestHandler clientSetResponseEntityRestHandler, ClientRestHandler clientResponseCompleteRestHandler, | ||
| ClientRestHandler clientErrorHandler) { | ||
| this.clientCaptureCurrentContextRestHandler = clientCaptureCurrentContextRestHandler; | ||
| this.clientSwitchToRequestContextRestHandler = clientSwitchToRequestContextRestHandler; | ||
| this.clientSendHandler = clientSendHandler; | ||
| this.clientSetResponseEntityRestHandler = clientSetResponseEntityRestHandler; | ||
| this.clientResponseCompleteRestHandler = clientResponseCompleteRestHandler; | ||
| this.clientErrorHandler = clientErrorHandler; | ||
| } | ||
|
|
||
| private HandlerChain newInstance() { | ||
| return new HandlerChain(clientCaptureCurrentContextRestHandler, clientSwitchToRequestContextRestHandler, | ||
| clientSendHandler, clientSetResponseEntityRestHandler, clientResponseCompleteRestHandler, clientErrorHandler); | ||
| } | ||
|
|
||
| HandlerChain setPreClientSendHandler(ClientRestHandler preClientSendHandler) { | ||
| this.preClientSendHandler = preClientSendHandler; | ||
| return this; | ||
| HandlerChain newHandlerChain = newInstance(); | ||
| newHandlerChain.preClientSendHandler = preClientSendHandler; | ||
| return newHandlerChain; |
There was a problem hiding this comment.
Wouldn't the changes here alone be enough to fix the problem? Asking because the change to ClientRequestContextImpl don't seem necessary (although the might be correct)
| @@ -15,16 +12,18 @@ | |||
| import org.jboss.resteasy.reactive.common.jaxrs.ConfigurationImpl; | |||
| import org.junit.jupiter.api.Test; | |||
|
|
|||
| import static org.junit.jupiter.api.Assertions.*; | |||
|
I am also interested to know if CI passed on your fork with this change |
| client.helloGet("Naruto"), | ||
| client.helloGet("Goku"), | ||
| client.helloGetUniExecutor()) | ||
| .combinedWith((s, s2, s3, s4) -> s + " and " + s2 + " and " + s3 + " and " + s4); |
There was a problem hiding this comment.
'combinedWith(io. smallrye. mutiny. tuples. Functions. Function4<T1,T2,T3,T4,O>)' is deprecated and marked for removal. Can you please use something else?
| Context ctxt = Vertx.currentContext(); | ||
| if (ctxt != null && VertxContext.isDuplicatedContext(ctxt)) { | ||
| this.context = ctxt; | ||
| } else { | ||
| Context current = client.vertx.getOrCreateContext(); | ||
| this.context = VertxContext.createNewDuplicatedContext(current); | ||
| } | ||
| Context current = client.vertx.getOrCreateContext(); | ||
| this.context = VertxContext.createNewDuplicatedContext(current); |
There was a problem hiding this comment.
I'm actually unsure of this. I would really like to know if the tests pass without this change
There was a problem hiding this comment.
It fails, I tried the test with the main changes.
There was a problem hiding this comment.
Well, I am not confident about this change at all, at least until somebody convinces me that it needs to be this way. I would like to understand why this is needed.
When combining multiple unis, the operationName field retains the last uni processed instead of retaining the value it was initialized with.