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

Per-endpoint instrumentation doesn't show up with overlapping API endpoints. #18

Open
unr-srs opened this issue Jul 13, 2020 · 2 comments

Comments

@unr-srs
Copy link

unr-srs commented Jul 13, 2020

Using lts-14.27.

The API type has overlapping endpoints plus a raw endpoint for Pandoc-generated API documentation. The application runs hoisted on a custom monad

type TheAPI = "try" :> "this" :> ...
                  :<|> "try" :> "that" :> ... 
                  :<|> "try" :> "those" :> ...
                  :<|> "lookup"
                  :<|> Raw

theAPI :: Proxy TheAPI
theAPI = Proxy

apiServer :: ServerT TheAPI CustomM
apiServer = ... :<|> ... etc

myApp config = serve theAPI $ hoistServer theAPI (nt cconfig) apiServer

where nt does the natural transformation via runReaderT and runLoggerT.

After a vanilla System.Remote.Monitoring.forkServer, I'm wrapping the application as follows

wrapWithEkg <- newStore >>= monitorEndpoints theAPI
runSettings settings (wrapWithEkg $ myApp cfg)

where runSettings does runTLS over a custom TCP port.

I can connect to EKG. The standard EKG metrics are there, but none of the Servant specific measurements show up.

@fiadliel
Copy link

fiadliel commented Aug 1, 2020

I've been using similar code to this in another project, and found that more complex routes also didn't show up. There is some code that I don't understand, and removing a check made reporting work better (thought it hasn't been tested for long, where "long" is more than a few requests on my laptop).

The code in question is:

instance ReflectMethod method => HasEndpoint (Verb method status cts a) where
    getEndpoint _ req = case pathInfo req of
        [] | requestMethod req == method -> Just (APIEndpoint [] method)
        _                                -> Nothing
      where method = reflectMethod (Proxy :: Proxy method)

(and similar instances).

The pathInfo check doesn't make sense to me, and I suspect it should be removed; the path alignment is already checked by captures and static path components. Also, the enumerateEndpoints function doesn't include the check in question.

This was an error.

@fiadliel
Copy link

fiadliel commented Aug 2, 2020

Sorry for the above; a few more hours show that I was wrong (the check proves that there aren't further segments in the path).

I've tried to reproduce at least whether TheAPI returns a value to getEndpoint. Would you be able to try locally, and see if it returns a correct value?

e.g. in the repl:

> :set -XTypeOperators -XDataKinds
> :{
>   type TheAPI = "try" :> "this" :> GetNoContent
>                     :<|> "try" :> "that" :> GetNoContent
>                     :<|> "try" :> "those" :> GetNoContent
>                     :<|> "lookup" :> GetNoContent
>                     :<|> Raw
> :}
> import Servant.Ekg
> import Data.Text
*Servant.Ekg Data.Text> getEndpoint (Proxy :: Proxy TheAPI) (defaultRequest{pathInfo = pack <$> ["try", "that"]})
Just (APIEndpoint {pathSegments = ["try","that"], method = "GET"})

All the tests I've tried here with the above API return the correct result, but I wonder if there's something different about your API.

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

2 participants