router
router copied to clipboard
koajs
Fix Router.match pathAndMethod to only include layers that have methods
@Gary-Osteen-Q2 Thanks for submitting a PR to fix #105. I have reviewed the changes. Just need a collaborator to review now. @niftylettuce do you have time to have a look at this? Let me know if there is anything I missed in my review, I am still new to this 😄
@niftylettuce: I believe this PR may be required to address this upstream issue: https://github.com/open-telemetry/opentelemetry-js-contrib/issues/222
@jmealo Thanks! I've always looked forward to giving back to open source eventually. I am elated to think that it might also fix some upstream issues!
Sorry I dropped the ball on this, is this ready to go? Can a few people look over this and ping me back?
@niftylettuce it is ready to go from code side. I don't know what else would be needed.
@3imed-jaberi I'm not sure I follow how it would make the update smooth. Logically the layer that gets added to pathOnly have no methods to match in the first place. Whereas pathNotMethod the layer did have a method and it didn't match. Path would then be too generic to me as I would expect others to come in and think that the path matched everything.
We should aim not to break this, which checks the path:
https://github.com/open-telemetry/opentelemetry-js-contrib/tree/master/plugins/node/opentelemetry-koa-instrumentation/src
@jmealo pathOnly is used internally by Router.prototype.match which is a @private method. No outside project should be looking at that variable directly. The ctx.matched gets set at the same place as before in Router.prototype.routes. I don't see how this should affect any other dependent project. Inside Router.prototype.routes the path and ctx.path are unchanged.
@Gary-Osteen-Q2: Sorry for my delayed reply. Your reasoning on a private method is correct in theory, however, the TC39 private methods and getter/setters proposal for JavaScript classes proposal is still a stage 3 draft. Typescript and JSDOC annotations can mark a method as private but JavaScript does not enforce it (yet).
As @3imed-jaberi: pointed out that this PR as implemented would need a MAJOR semver bump because it's an incompatible/breaking change.
I saw an actual monkey patch in the wild that wrapped internal methods of koa-router to set the operation name to the route on outgoing telemetry because there was no way to do it with the public API. npm is full of instrumentation libraries that monkey patch "private" methods.
Instrumentation developers can't really wait/lobby for each upstream projects to change the public API to expose its internals for what they'll consider a niche use case and their users needed better instrumentation yesterday. Since JavaScript doesn't have private methods, we have to tread lightly until there's a TypeScript/ES-Next native ecosystem. Ideally library authors will provide their own instrumentation with time. If that's what you're into you Deno is trying to be that thing.
TLDR: @niftylettuce If we change the path back to pathOnly this is good to go with a PATCH release. If not, this needs to be a MAJOR.
@JacobMGEvans @Gary-Osteen-Q2 I think it is inferred that path is the same as pathOnly (from naming point of view) - is there any reason other than naming to have it renamed? Or perhaps have both and add the new one and use that internally? I would keep it backwards compatible!
@Gary-Osteen-Q2 can you please rebase with the master branch to resolve the conflict and go on here
@Gary-Osteen-Q2 could you please rebase with the latest release main changes, I will take care to ship this fix on your next release whatever major or minor release ;) !