fix(instrumentation-express)!: remove @types/express from dependencies

[david-luna]

Which problem is this PR solving?

@types/express is set as a dependency of express instrumentation therefore it is installed in any project using this package. This is causing compatibility issues.

Short description of the changes

• moved @types/express to devDependencies
• made ExpressRequestInfo generic so it can be typed by consumers
• update README & references to config options
• move from deprecated SpanAttributes type to Attributes

Closes: #1787

[codecov]

Codecov Report

Merging #1804 (51015d0) into main (8f2a195) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1804   +/-   ##
=======================================
  Coverage   91.49%   91.49%           
=======================================
  Files         144      144           
  Lines        7388     7388           
  Branches     1474     1474           
=======================================
  Hits         6760     6760           
  Misses        628      628           

Files	Coverage Δ
...try-instrumentation-express/src/instrumentation.ts	100.00% <100.00%> (ø)
...etry-instrumentation-express/src/internal-types.ts	100.00% <ø> (ø)
...opentelemetry-instrumentation-express/src/utils.ts	97.61% <ø> (ø)

[david-luna]

Note for reviewer: type argument T defaults to any so anyone already using requestHook and spanNameHook will still work at the type level. The unknown type was considered but it will trigger typing errors within these hooks.

[JamieDanielson]

Just to confirm I understand correctly - by keeping it as any, we're sort of opting out of type checking and leaving it up to the consumer to type as needed... but we avoid breakage for existing users who already have this in their codebase (I did see this error in testing local changes for unknown).

So with this, nothing needs to be changed for existing TypeScript users, and new users will just need to add this extra type on their own, which may not be asking much because they probably already use @types/express.

Does that sound like an accurate statement?

[david-luna]

Theree is a small detail for existing TS users. The type of request property will become of type any. Therefore TS will not provide autocompletion and type checks for it and its sub-properties. This may not be nice to existing users.

To mitigate that we may create our own default type. I've already tried to copy express' request type but there are many links to other types making it a big chunk of code just for typing.

Maybe we could agree on a subset of the request api but this come with the risk of breaking the build of existing TS users of they access a property not defined on this default type.

[david-luna]

@pkanal @JamieDanielson could you have a look please?

[blumamir]

This looks like a great workaround for an annoying problem. I guess it can be adopted in other instrumentations as well?

If you have some free time, it could be awesome to also document this in the guidelines

[JamieDanielson]

Thanks for this! I've left a question to ensure I'm understanding the change properly, but it seems like the right move here and probably other packages as Amir mentioned as well.

I'll want to update the example (as I'm using it I'm really becoming aware of how outdated it is 😅) to show these changes, but can do as a separate PR once this lands).

[JamieDanielson]

Just to confirm I understand correctly - by keeping it as any, we're sort of opting out of type checking and leaving it up to the consumer to type as needed... but we avoid breakage for existing users who already have this in their codebase (I did see this error in testing local changes for unknown).

So with this, nothing needs to be changed for existing TypeScript users, and new users will just need to add this extra type on their own, which may not be asking much because they probably already use @types/express.

Does that sound like an accurate statement?

[david-luna]

This looks like a great workaround for an annoying problem. I guess it can be adopted in other instrumentations as well?

If you have some free time, it could be awesome to also document this in the guidelines

We can bring this to the next SIG and if there is consensus I can start right away :)

[JamieDanielson]

🤔 I'm still having a tough time with using any here, and also see what you mean about the problems we run into with trying our own subset of the Request type.

This is an older thread but I see it referenced in various places and agree with the sentiment:

Given that breaking consumers is a worse problem than slightly-larger packages, we've made --save the default in our documentation.

Much of this conversation (and linked issues/PRs in other libraries) is around the idea that although types packages are often dev dependencies, these types are explicitly used to interact with this library and so need to be available to consumers of the library.

What if we tried moving this to an optional peer dependency (peerDependenciesMeta) for @types/express? TypeScript users do need this package anyway, so this makes it more explicit.

[Flarna]

TypeScript users do need this package anyway

That's only true if one uses express. The instrumentation is part of the auto-instrument package therefore users get it even they don't use express. It would be strange to force them to install @types/express.

Regarding a general guide:
For express it's a dedicated @types package as of now which is quite small on install, no runtime size and usually no further dependencies. For other packages (e.g. @grpc/grpc-js, fastify) types are included in the package itself. In this case it would be needed to depend on the package itself. In special if several major versions are supported by the instrumentation it's hard to find a single one which offers the types needed across the whole range.

Therefore the usual rule is to avoid such dependencies by not using any types of the instrumentee in the instrumentation.

[JamieDanielson]

This is a complicated issue when considering the implications of using the express instrumentation package on its own... but especially when considering both auto-instrument package and the express instrumentation package.

It would be strange to force them to install @types/express

Understood. But this logic also shows that the auto-instrument package includes express instrumentation even though a non-express consumer doesn't need that either - along with all the other instrumentations that are included in the auto-instrument package that they may not need. I don't know that I agree with making an already bloated package slightly smaller for users who don't benefit from these types anyway, while making manual instrumentation/setup harder for users who do benefit from these types.

The ideal solution for the auto-instrument package is to make it install only those instrumentations that are necessary, with the ability to disable/exclude unnecessary packages like express-instrumentation - I guess similar to the python bootstrap. The ideal solution for this type dependency change doesn't really exist... I can absolutely see the argument for keeping it in dev and documenting the use case (as is done with this PR). But I also see that adding more friction and now having less type-safety for an end user, resulting in a poor experience. I may be blowing that out of proportion, but that is my hesitation here. The fact that types are not supplied from express itself does make this more challenging than those other packages noted.

[Flarna]

auto instrumentation package follow the typical APM approach to "just install the tool" and it takes care of everything. on default the only config needed should be exporter details. If defaults of all instrumentations and SDK are choosen reasonable (maybe even dependent on environment like AWS lambda vs normal host,...) only a few power user should have the need to use hooks.
so far the ideal world :o)

I don't know that I agree with making an already bloated package slightly smaller for users who don't benefit from these types anyway, while making manual instrumentation/setup harder for users who do benefit from these types.

It's not that much about size (at least not in the case where it is about a types only package).
It's not the instrumentation which should select the @types version - it's the end user along with the express version they use.

We could keep it in dependencies and use * as version to match anything and choose latest on default. But that cries for autobreaking of the CI here. Maybe not that a hot potato for express as API is quite stable but I doubt * is a good idea as general rule.

I guess once express decides to include types it's clear that we don't want it as dependency. So removing types could be also seen as preparation for this.

The ideal solution for the auto-instrument package is to make it install only those instrumentations that are necessary

I don't think this is supported by NPM.

[JamieDanielson]

I have now read through so many issues and PRs about this (mostly linked from that types-publisher issue) and... phew 😅 . This is a hotly debated topic, and it doesn't seem like there's a definitive right answer that really works for everyone.

It's not the instrumentation which should select the types version - it's the end user along with the express version they use.

I totally agree with this.

We could keep it in dependencies and use * as version to match anything and choose latest on default. But that cries for autobreaking of the CI here. Maybe not that a hot potato for express as API is quite stable but I doubt * is a good idea as general rule.

I've also considered a larger range for version matching, though yeah * does seem excessive.

🤔 So as I understand it, the general guidance seems to be to include the types as a dependency only if it's exported, which ExpressRequestInfo is. This is why we had to make the ExpressRequestInfo generic, and consumers of this package that directly use the ExpressRequestInfo can type their own, as Amir showed in a previous comment. Those without types (not installed or using JS) will lose autocompletion and IDE help for this specific functionality, but that is not as much due to the instrumentation as it is due to express not having built-in types.

This may be the right move. I'm not sure how easily it will be implemented in other instrumentations as I'm not sure how intertwined those types are. But if this helps reduce fragility that seems good too. Also PS I'm sorry I'm seeing this on the PR instead of helping with the discussion earlier on the issue.

[JamieDanielson]

This looks like a great workaround for an annoying problem. I guess it can be adopted in other instrumentations as well?
If you have some free time, it could be awesome to also document this in the guidelines

We can bring this to the next SIG and if there is consensus I can start right away :)

@david-luna General consensus in the SIG meeting today was that this should be documented and attempted to be adopted in other instrumentations 👍 cc @blumamir

[david-luna]

This looks like a great workaround for an annoying problem. I guess it can be adopted in other instrumentations as well?
If you have some free time, it could be awesome to also document this in the guidelines

We can bring this to the next SIG and if there is consensus I can start right away :)

@david-luna General consensus in the SIG meeting today was that this should be documented and attempted to be adopted in other instrumentations 👍 cc @blumamir

Thanks @JamieDanielson I'll prepare a PR to add this approach to the docs :)