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

Make interface type available to providers #121

Closed
pavolloffay opened this issue Sep 14, 2018 · 16 comments
Closed

Make interface type available to providers #121

pavolloffay opened this issue Sep 14, 2018 · 16 comments
Assignees
Milestone

Comments

@pavolloffay
Copy link
Member

pavolloffay commented Sep 14, 2018

Use case:
Provider could use annotations on the interface to configure its behavior. For example tracing integration could be disabled if @Traced(false) is added on the interface. Or it could add custom meta-data - tags specified in that annotation to the span created in the client.

This is blocking eclipse/microprofile-opentracing#102

@kenfinnigan
Copy link
Contributor

Where would such information be made available?

Not sure I'm understanding what's required and how.

@pavolloffay
Copy link
Member Author

Let's say I have this client:

@Traced
@TracedTags({"foo": "bar"})
@Path("/")
interface Service {
  @GET
  @Path("/get")
   get();
}

It would mean that the client is traced and the tracing integration would add foo:bar to the span created for the method invocation. Generally speaking there could be any annotation, important is that the Service.class should be available to the provider/feature class which is added to the client.

The Service.class interface could be made available in
javax.ws.rs.core.Coonfiguration.getProperty("mp.rest.client.interface"). Which is accessible in Feature and ClientRequestContext.

@andymc12
Copy link
Contributor

andymc12 commented Sep 19, 2018

Talking with @pavolloffay - it sounds like the ideal solution would be to have a RestClientListener that is invoked with the client itself is created (as opposed to the RestClientBuilderListener which is invoked when the builder is created). The RestClientListener's onNewClient method should include the rest client interface class - something like:
void onNewClient(Class<?> clientInterface, RestClientBuilder builder)

The RestClientListener implementations should be registered using the ServiceLoader pattern - similar to RestClientBuilderListener.

@andymc12 andymc12 added this to the 1.2 milestone Sep 19, 2018
@pavolloffay
Copy link
Member Author

@andymc12 I have more thoughts on the issue.

It would be great if a provider(e.g. filter) would have access to the method of the interface(which means it has an access to the interface itself). Then an annotation can be used to change a behavior of the method e.g. @Traced(false) on the specific method rather than on the whole client.

@andymc12
Copy link
Contributor

@pavolloffay @kenfinnigan I think this request makes sense. What do you guys think of something like:

public interface RestClientRequestContext extends ClientRequestContext {

    Method getInvokedMethod();
}

...

public interface RestClientRequestFilter {

    void filter(RestClientRequestContext requestContext) throws IOException;
}

@pavolloffay
Copy link
Member Author

If I get it correctly then users can use ClientRequestFilter or RestClientRequestFilter to basically do the same thing? Shouldn't we instead provide the Method in request attributes under some key? Or keep proposed RestClientRequestContext which would be passed into jaxrs ClientRequestFilter. Implementation could use instanceof on the context fo find out the type.

From these two putting the method into attributes seems easier to implement.

@andymc12
Copy link
Contributor

Sure, we could put the Method object in the properties obtainable by ClientRequestContext.getProperty(...) - something like org.eclipse.microprofile.rest.client.invokedMethod.

@pavolloffay
Copy link
Member Author

It sounds good, Will it be done in 1.2?

@andymc12
Copy link
Contributor

andymc12 commented Oct 3, 2018

@pavolloffay - yes, that's the plan. Thanks!

andymc12 added a commit to andymc12/cxf that referenced this issue Oct 3, 2018
This partially resolves issue
[121](eclipse/microprofile-rest-client#121)
for MicroProfile Rest Client 1.2 - enabling `ClientRequestFilter`s to
access the Rest Client interface method being invoked.
andymc12 added a commit to andymc12/cxf that referenced this issue Oct 3, 2018
This partially resolves issue
[121](eclipse/microprofile-rest-client#121)
for MicroProfile Rest Client 1.2 - enabling `ClientRequestFilter`s to
access the Rest Client interface method being invoked.
@andymc12 andymc12 self-assigned this Oct 18, 2018
@andymc12
Copy link
Contributor

@pavolloffay - are you ok with me closing this issue? There are effectively two changes here:

  1. A new RestClientListener API which alerts when an actual client impl is creating (similar to the previous RestClientBuilderListener).
  2. A new property that provides the java.lang.reflect.Method of the invoked Rest Client interface method available via the ClientRequestContext on client request/response filters.

@pavolloffay
Copy link
Member Author

@andymc12
Copy link
Contributor

When do you plan to cut a release with this changes? I would like to start building OT integration with this.

@pavolloffay Our tentative plan is to release MP Rest Client 1.2 in 4Q2018. I say tentative, because the last I heard, the next release of the overall MicroProfile spec is scheduled for 1Q2019 - so it's possible that MP Rest Client 1.2 will not be released until then.

In the mean time... I don't know what the policy for "early-access" releases are. At this point, we are not in good shape to cut a release candidate, but maybe we could cut some sort of alpha or beta release? @kenfinnigan what do you think?

@pavolloffay
Copy link
Member Author

I would like to use this feature in the next OT version, which should be also included in the overall spec. If you can cut a release earlier it would give us more time to develop and test. If you cut it just before the overall release there will be a short time for us to integrate.

@pavolloffay
Copy link
Member Author

@andymc12 @kenfinnigan any news on the release schedule?

@andymc12
Copy link
Contributor

@pavolloffay sorry for the slow reply - yes, we just published a milestone release with these changes included. You can see more info at the MP Google Group post:
https://groups.google.com/forum/#!topic/microprofile/y_yS1qtpiJQ

@pavolloffay
Copy link
Member Author

This is great thank you! This ticket can be closed.

rmannibucau pushed a commit to rmannibucau/cxf that referenced this issue Mar 18, 2019
This partially resolves issue
[121](eclipse/microprofile-rest-client#121)
for MicroProfile Rest Client 1.2 - enabling `ClientRequestFilter`s to
access the Rest Client interface method being invoked.
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

No branches or pull requests

3 participants