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

Fix bad resource name with Grape::API when used in Grape 1.2.0 #639

Merged
merged 1 commit into from
Nov 28, 2018

Conversation

delner
Copy link
Contributor

@delner delner commented Nov 26, 2018

Grape released version 1.2.0 on 11/26/18 which broke our CI builds. In this new version, Grape deprecated the use of Grape::API directly.

Applications that are still using Grape::API after 1.2.0 will not be able to see the class name of the API in their trace resource names. This is because these well-known types are coerced into anonymous classes as noted in this issue. Users who experience this, as a workaround, should upgrade their base class from Grape::API to Grape::API::Instance as described here, e.g.

# Old
class MyApi < Grape::API; end

# New
class MyApi < Grape::API::Instance; end

To fix our CI build, we applied this change. To prevent misbehavior of tracing for users who neglect to undertake this upgrade, we hide the class name and tag, so as to avoid meaningless resource names like #<Class:0xXXXXXX>#success, in favor of #success.

UPDATE

Although the Instance object that's provided is an anonymous class, it turns out it is possible to resolve the name of the original class using the base method. I've updated the PR accordingly which means we now get the nice class name in resource for Grape 1.2.0 implementations that still use Grape::API, giving us backwards compatibility.

@delner delner added bug Involves a bug integrations Involves tracing integrations labels Nov 26, 2018
@delner delner self-assigned this Nov 26, 2018
@delner delner added this to the 0.17.3 milestone Nov 26, 2018
pawelchcki
pawelchcki previously approved these changes Nov 27, 2018
@dblock
Copy link

dblock commented Nov 27, 2018

Given that this is now potentially mounted multiple times, is this change sufficiently differentiating between the two different mounted endpoints? I think the correct fix would be getting the API name + some instance ID (because there may be multiple mounted instances).

I commented ruby-grape/grape#1825 (comment) on whether this is needed in Grape proper.

@delner
Copy link
Contributor Author

delner commented Nov 27, 2018

@dblock Yeah, maybe an instance ID would be useful. I'm not yet very familiar with the concept of mounted instances in Grape: how do different instances of the same API differ? Is it minor configuration or are there more substantial differences between instances of the same API?

@dblock
Copy link

dblock commented Nov 27, 2018

@dblock Yeah, maybe an instance ID would be useful. I'm not yet very familiar with the concept of mounted instances in Grape: how do different instances of the same API differ? Is it minor configuration or are there more substantial differences between instances of the same API?

Technically, before we mounted a static module, now we mount instances of an API.

Logically you can define /foo in an API, but mount it as /v1/foo and /v2/foo, so the full path is very different. In previous versions of Grape this was not possible with the same code and you had to duplicate everything.

Note that there's a case where the path is the same, but the API switches by version. So /foo with an Accept header for v1 will give you an instance of the API mounted for V1, while /foo with an Accept header for v2 will give you an instance of the API mounted for V2. They may differ quite significantly obviously.

@delner
Copy link
Contributor Author

delner commented Nov 27, 2018

@dblock I see, yeah, that could be very significant. We'd probably want to factor in that full path (in your example) to differentiate between the different instances because those represent fundamentally different kinds of operations (e.g. calling between versions 1 and 2.)

@delner delner merged commit bac0930 into master Nov 28, 2018
@delner delner deleted the fix/grape_api_resource_name branch November 28, 2018 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Involves a bug integrations Involves tracing integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants