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

fix(express): make rpcMetadata.route capture the last layer even when if the last layer is not REQUEST_HANDLER #1620

Merged
merged 15 commits into from
Aug 13, 2023

Conversation

chigia001
Copy link
Contributor

@chigia001 chigia001 commented Aug 1, 2023

Which problem is this PR solving?

Short description of the changes

  • Update the condition to match the previous implementation for rpcMetadata.route
  • Move this block bellow all the ignore flow, which make it the same as old implementation.

@chigia001 chigia001 requested a review from a team August 1, 2023 08:54
@codecov
Copy link

codecov bot commented Aug 3, 2023

Codecov Report

Merging #1620 (a5aec4a) into main (7d4b13e) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1620      +/-   ##
==========================================
- Coverage   91.79%   91.79%   -0.01%     
==========================================
  Files         139      139              
  Lines        7117     7115       -2     
  Branches     1431     1429       -2     
==========================================
- Hits         6533     6531       -2     
  Misses        584      584              
Files Changed Coverage Δ
...try-instrumentation-express/src/instrumentation.ts 100.00% <100.00%> (ø)

Copy link
Member

@blumamir blumamir left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks again 🥇

Added a question regarding the test for this change

next();
});

app.use('/bare_middleware', (req, res) => {
Copy link
Member

Choose a reason for hiding this comment

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

This test should assert that the route is still captured when

type === ExpressLayerType.REQUEST_HANDLER

This type will be set when layer.name === 'bound dispatch'

  } else if (layer.name === 'bound dispatch') {
    return {
      attributes: {
        [AttributeNames.EXPRESS_NAME]: layerPath ?? 'request handler',
        [AttributeNames.EXPRESS_TYPE]: ExpressLayerType.REQUEST_HANDLER,
      },
      name: `request handler${layer.path ? ` - ${layerPath}` : ''}`,
    };

Is using the app.use triggers a layer with name 'bound dispatch'?

Copy link
Contributor Author

@chigia001 chigia001 Aug 13, 2023

Choose a reason for hiding this comment

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

  const router = express.Router();

  router.use("/router_use", () => {}); // #3
  router.get('/tada/, () => {}); // #2

  app.use('/toto', router); // #1
  app.use('/bare_use', () => {}); // #3

  app.get('/bare_get', () => {}); // #2

#1, the middleware that bound app.use/router.use with Router object is 'router' layer

#2 is bound dispatch, ie, route handler with HTTP method name. This is for both Express's App and Express's Router object. This is something our unit test cover most of the time. Express's route handle will match to the end of URL.

#3 is the last case, IE middleware layer, where app.use/router.use have generic handler. It can be with or without the route's parameter .use will only need to match the start of the url.

Most of our test with app.use/router.use is for common's middleware use case, that don't have layername at the begining.

This new test for specificly app.use with route layer.

Copy link
Contributor Author

@chigia001 chigia001 Aug 13, 2023

Choose a reason for hiding this comment

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

I add additional test for these 2 case, as I don't see we have specific test for them

  router.use("/router_use", () => {}); // #3
  app.get('/bare_get', () => {}); // #2

internally I think they are the same with case that we already cover (app.use === router.use and app.get === router.get) but just to make sure.

@blumamir blumamir merged commit eeda32a into open-telemetry:main Aug 13, 2023
@chigia001 chigia001 deleted the fix-1618-express-rpcMetadata branch August 13, 2023 06:44
@dyladan dyladan mentioned this pull request Aug 13, 2023
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.

Express instrumentation 0.33.0 breaks handling http_route metric attribute
5 participants