Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

DRAFT: Close Middleware Spans Early #1

Draft
wants to merge 142 commits into
base: honeycomb-otel
Choose a base branch
from

Conversation

jfhbrook-at-work
Copy link
Owner

Eric suggested that it would be cool if we could close the middleware spans prior to the next middleware getting called. In other words, instead of

GET /
|-mw: x
  |- mw: route
    |- handler: handler

this:

GET /
|-mw: x
|-mw: route
|-handler: handler

This would make traces a lot more compact and give a better idea at a glance of how long the entry for the middleware takes. For exit, we could do another trace on the back, or assume that work is usually short-lived and emit a span event on exit instead.

As you can hopefully see from the diff, this is EXTREMELY hairy. What you won't see from the diff is that the tests currently fail - the test middleware's context gets added to the HTTP span, not the test middleware span. Alas!

I didn't want to push on this too far + instead focus on what we have now, but I thought it was a good idea and wanted to post my notes.

Josh Holbrook and others added 30 commits January 10, 2022 17:07
The url structure is `/v${versionNumber}/`, so these errors produce 404s
enforceInvariants()
enforceInvariants(),
// {% if honeycomb %}
endSpan(mw),
Copy link
Owner Author

Choose a reason for hiding this comment

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

The bug is that endSpan needs to be spliced after the mw, not before - but finishing this thought will require refactoring how it terminates at the router.

@jfhbrook-at-work
Copy link
Owner Author

Since implementing this busted sketch, we learned that there's an alternate bind function which will make implementing this quite a bit easier than using with:

https://medium.com/aspecto/context-managment-in-opentelemetry-nodejs-e5cddf22a09e

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant