fix(express): fix wrong rpcMetadata.route value with router.use(<midd…

[biuboombiuboom]

What version of OpenTelemetry are you using?

opentelemetry: 1.4.0
node: 16.14.2

What version of Node are you using?

What did you do?

    const app = express();
    const router = express.Router();
    app.use('/users', router);
    router.get('/:userId', (req, res) => {
      res.status(200).end('user-'+req.params.userId);
    });
    router.use(customMiddleware);
    const bookRouter = express.Router({ mergeParams: true });
    router.use('/:userId/books', bookRouter);
    bookRouter.get('/:bookId', (req, res) => {
      setImmediate(() => {
        res.status(200).end('book-' + req.params.bookId);
      });
    });       

request http://localhost:8080/proxy/v1/bar/1

What did you expect to see?

rpcMetadata.route='/users/:userId/books/:bookId'

What did you see instead?

rpcMetadata.route='/:userId/books/:bookId'

[chigia001]

@biuboombiuboom can you try remove this condition in
to see if it still pass all your test?

My bad, different issue.

[codecov]

Codecov Report

Merging #1615 (a66a4a4) into main (154b30b) will not change coverage.
The diff coverage is n/a.

❗ Current head a66a4a4 differs from pull request most recent head 727e729. Consider uploading reports for the commit 727e729 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1615   +/-   ##
=======================================
  Coverage   96.06%   96.06%           
=======================================
  Files          14       14           
  Lines         914      914           
  Branches      199      199           
=======================================
  Hits          878      878           
  Misses         36       36           

[chigia001]

I think we should simlify the test setup.

[chigia001]

so we are over pop item right,

If layerPath is undefined, we don't append new record _LAYERS_STORE_PROPERTY. But when next is call, in the current implementation we still pop it.

[chigia001]

This customMiddleware layer is the one that make we over remove one layer.

[chigia001]

@biuboombiuboom the current implementaion can potential lead to the same issue here right?
https://github.com/open-telemetry/opentelemetry-js-contrib/pull/1615/files#diff-81c3e13c3dac53e0dd6aabf6dc165305350eb522a85a2561f9ee51d9249011e7L209

[chigia001]

@biuboombiuboom I think we can refactor storeLayerPath to make it more strict on push/pop action. It also prevent instrumentation.ts from knowing of this symbol property.

export const storeLayerPath = (request: PatchedRequest, value?: string): {route: string, popLayerPath: () => void} => {
  let requestRouteStack: string[]
  if (Array.isArray(request[_LAYERS_STORE_PROPERTY]) === false) {
    requestRouteStack = [];
    Object.defineProperty(request, _LAYERS_STORE_PROPERTY, {
      enumerable: false,
      value: requestRouteStack,
    });
  } else {
    requestRouteStack = request[_LAYERS_STORE_PROPERTY] as string[]
  }
  if (value !== undefined) {
    requestRouteStack.push(value)
  }

  const stackLength = requestRouteStack.length

  const route = requestRouteStack
          .filter(path => path !== '/' && path !== '/*')
          .join('');
  let alreadyPopStack = false
  return {
    route,
    popLayerPath: () => {
      if (value !== undefined) {
        return
      }
      if (requestRouteStack.length === stackLength && alreadyPopStack === false) {
        requestRouteStack.pop()
        alreadyPopStack = true
      } else {
        diag.error(
          'express instrumentation: pop route stack incorrectly'
        );
      }
    }
  }
};

[biuboombiuboom]

@biuboombiuboom I think we can refactor storeLayerPath to make it more strict on push/pop action. It also prevent instrumentation.ts from knowing of this symbol property.

export const storeLayerPath = (request: PatchedRequest, value?: string): {route: string, popLayerPath: () => void} => {
  let requestRouteStack: string[]
  if (Array.isArray(request[_LAYERS_STORE_PROPERTY]) === false) {
    requestRouteStack = [];
    Object.defineProperty(request, _LAYERS_STORE_PROPERTY, {
      enumerable: false,
      value: requestRouteStack,
    });
  } else {
    requestRouteStack = request[_LAYERS_STORE_PROPERTY] as string[]
  }
  if (value !== undefined) {
    requestRouteStack.push(value)
  }

  const stackLength = requestRouteStack.length

  const route = requestRouteStack
          .filter(path => path !== '/' && path !== '/*')
          .join('');
  let alreadyPopStack = false
  return {
    route,
    popLayerPath: () => {
      if (value !== undefined) {
        return
      }
      if (requestRouteStack.length === stackLength && alreadyPopStack === false) {
        requestRouteStack.pop()
        alreadyPopStack = true
      } else {
        diag.error(
          'express instrumentation: pop route stack incorrectly'
        );
      }
    }
  }
};

greate.
Is there a lack of processing for the ignore layer?

[blumamir]

@biuboombiuboom Let me know when this is ready for review