-
Notifications
You must be signed in to change notification settings - Fork 552
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(express): span name if middleware on nested router is used #2682
base: main
Are you sure you want to change the base?
Conversation
That is a pretty significant improvement, good job! |
Hey there, just wanted to share that I've tried out the changes from this PR by tweaking the OTEL libraries in node_modules for our app, and it sorted out the issues we were facing with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some feedback but I don't feel it's important enough to warrant not approving. Thanks for the contribution !
* @param [value] the value to push into the array | ||
*/ | ||
export const storeLayerPath = (request: PatchedRequest, value?: string) => { | ||
export const storeLayerPath = ( | ||
request: PatchedRequest, | ||
value?: string | ||
): { isLayerPathStored: boolean } => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is extremely minor but I can't really think of any other information that could be returned by this, so personally I'd rather see this skip the object and just return a boolean, the meaning of which can be documented in the docstring.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2682 +/- ##
=======================================
Coverage 92.41% 92.41%
=======================================
Files 171 171
Lines 8150 8152 +2
Branches 1653 1653
=======================================
+ Hits 7532 7534 +2
Misses 618 618
|
Which problem is this PR solving?
Short description of the changes
storeLayerPath
function, which tells the caller wether a new item has been added to the layer store. This is needed, because we only want to pop from thereq[_LAYERS_STORE_PROPERTY]
list if we added something to it. Popping from this list in case we did not push anything to it will result in popping a parent entry, which will skew the layers store. Hence this unnecessary popping will result in a malformed http.route tag.