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

Add support for Grails #2512

Merged
merged 6 commits into from
Mar 9, 2021
Merged

Add support for Grails #2512

merged 6 commits into from
Mar 9, 2021

Conversation

laurit
Copy link
Contributor

@laurit laurit commented Mar 5, 2021

  • set server span name to /context/controller/action
  • create span for grails controller invocations

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

set server span name to /context/controller/action

Is it possible to get and use the url-based route instead?

@iNikem
Copy link
Contributor

iNikem commented Mar 8, 2021

set server span name to /context/controller/action

Is it possible to get and use the url-based route instead?

I think that span name based on controller/action is a good one. But /context/controller/action mixes url-based name (context) with source code based (controller/action). Also it would be more idiomatic to have dot as separator: conroller.action

I don't have a good suggestion atm.

@trask
Copy link
Member

trask commented Mar 8, 2021

/context/controller/action mixes url-based name (context) with source code based (controller/action).

I don't have a good suggestion atm.

Ya, me either, that's why I'm hoping there's a reasonable way to get and use the url-based route.

Also it would be more idiomatic to have dot as separator: controller.action

Interestingly the rpc semantic conventions use classname/methodname

@laurit
Copy link
Contributor Author

laurit commented Mar 8, 2021

It might be better to just accept that span name is something that identifies a trace. How exactly will depend on the circumstances, it might not be possible to always get a result that immediately makes sense.
For rest urls it is easy, there is a path that nicely describes the current operation, we only need to get rid of parameters in path. Using className.methodName is not that great, what if application has multiple classes with same name in different packages or there are multiple applications that have a class with same name? In case of paths context path helps to distinguish different applications, maybe we should use /contextPath/className.methodName instead? The downside would be that it is a bit less obvious how span name is constructed and if you only have a single application then context path is redundant anyway. Also when using class name it might happen that the actual controller class has been proxied and we end up with a name like $Proxy.foo. Sometimes neither path not controller are a good fix, for example in jax-ws framework integration I appended operation name to span name, this really is not a part of path, just something that is inside the xml that was submitted to the web service.
Grails tutorials use the following mapping

package demo

class UrlMappings {

    static mappings = {
        "/$controller/$action?/$id?(.$format)?"{
            constraints {
                // apply constraints here
            }
        }

        "/"(view:"/index")
        "500"(view:'/error')
        "404"(view:'/notFound')
    }
}

So I'd assume that /context/controller/action is actually a path that a lot of grails applications use because that seems to be the convention. For test I used a different mapping scheme just to have endpoints where HttpServerTest expects them to be. I agree that all of this is not ideal, but this seemed to be the easiest way to get span names that identify the operation. I guess could also use controllerClass.methodName but that doesn't have context path and then server span would have the same name as nested handler span. It should be possible to also get the matched pattern which for our tests would produce good names like /context/success/* but fail miserably for the mapping from tutorial and produce something like /context/*/*/*. With some effort it might be possible to replace controller and action wildcards in pattern with actual names and translate parameter wildcards back to variable names.

@iNikem
Copy link
Contributor

iNikem commented Mar 8, 2021

@laurit convinced me on the status quo

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"/$controller/$action?/$id?(.$format)?"

oh my, I see why this is problematic

With some effort it might be possible to replace controller and action wildcards in pattern with actual names and translate parameter wildcards back to variable names.

agree status quo is good, thanks for the explanations 👍

@iNikem iNikem merged commit 7013376 into open-telemetry:main Mar 9, 2021
@laurit laurit deleted the grails branch March 9, 2021 12:08
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

Successfully merging this pull request may close these issues.

None yet

5 participants