chore: moves express examples into its package to establish pattern

[andyfleming]

Problem this solves

Examples get out of date quickly since they are at the top-level of this repository. There's no validation that they work correctly with the latest version of instrumentation packages.

Changes in this PR

• Establishes new pattern of publishing examples in their respective instrumentation package directories (as discussed at the OTEL js SIG meeting on March 2nd).
• Moves example for express to the express instrumentation directory
• Converts express example to TypeScript to support a build step in CI as a technique for testing/validation of the example.
• Adds documentation in CONTRIBUTING.md about how to organize examples.
• Adds a README.md to the examples folder to note the migration (and can be used to document other standard examples in that folder).

Testing approach

For validation of the instrumentation example code, I propose we leverage TypeScript compilation. It's not as exhaustive as some sort of functional tests, but I think that would be too costly at this point.

I've implemented that via the npm run --if-present compile:examples command in CI.

We could later establish a pattern for CI to check for a "test:examples" script in the package.json of each instrumentation package to run (if present) in addition to the basic TypeScript compilation.

Additional Notes

It was discussed that we could keep the top-level examples directory for 2 reasons:

1. To leave it as a home for examples that have not yet been migrated to their respective package directories.
2. To provide a basic instrumentation example and a README that guides users to the examples in the individual instrumentation packages.

Checklist

• Ran npm run test-all-versions for the edited package(s) on the latest commit if applicable.

[rauno56]

I support moving examples under respective packages and happy to see someone is getting the ball rolling.

How do you think converting the examples to Typescript will affect the benefit non-TS users will get from them?

A previous discussion on this: #552

[dyladan]

I support moving examples under respective packages and happy to see someone is getting the ball rolling.

How do you think converting the examples to Typescript will affect the benefit non-TS users will get from them?

A previous discussion on this: #552

I think the benefit of having the examples type-checked and therefore easier to notice when they are outdated will outweigh the disadvantage of presenting typescript to a non-typescript user.

[Flarna]

A bit of topic regarding this specific PR. But what about adding an InMemorySpanExporter, run the samples in CI and verify that they create the required N spans?

[dyladan]

A bit of topic regarding this specific PR. But what about adding an InMemorySpanExporter, run the samples in CI and verify that they create the required N spans?

I wonder how hard it would be to do that in a way where the example isn't unnecessarily complex for new users who are just trying to figure out how to use the instrumentation?

[vmarchaud]

A bit of topic regarding this specific PR. But what about adding an InMemorySpanExporter, run the samples in CI and verify that they create the required N spans?

Shouldn't the tests already verify that anyway ?

[Flarna]

As far as I know the samples are not executed at all in CI. The tests of e.g. express instrumentation verify that the instrumentation works but not that the example works.

[andyfleming]

I added instructions for migrating examples in the examples/README file. I also created an example of what an issue would look like for a given package that needs converted. See the example issue for koa instrumentation here.

[andyfleming]

I believe this PR is ready to go now 👍

[rauno56]

I believe this PR is ready to go now +1

Sorry, I merged another PR that caused conflicts here.

[codecov]

Codecov Report

Merging #939 (891f621) into main (c5b9356) will increase coverage by 0.58%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #939      +/-   ##
==========================================
+ Coverage   95.91%   96.50%   +0.58%     
==========================================
  Files          13       19       +6     
  Lines         856     1086     +230     
  Branches      178      230      +52     
==========================================
+ Hits          821     1048     +227     
- Misses         35       38       +3     

Impacted Files	Coverage Δ
...opentelemetry-instrumentation-express/src/types.ts	100.00% <0.00%> (ø)
...tapackages/auto-instrumentations-node/src/utils.ts	97.91% <0.00%> (ø)
...nstrumentation-express/src/enums/AttributeNames.ts	100.00% <0.00%> (ø)
...trumentation-express/src/enums/ExpressLayerType.ts	100.00% <0.00%> (ø)
...try-instrumentation-express/src/instrumentation.ts	99.25% <0.00%> (ø)
...opentelemetry-instrumentation-express/src/utils.ts	97.36% <0.00%> (ø)

[dyladan]

lint also fails on the markdown lint check

[andyfleming]

Ok, lint should be resolved and latest changes from main merged.

[rauno56]

Why eslint-ignore the examples?

[blumamir]

LGTM
Thanks @andyfleming for doing this

The CI currently fails due to unrelated issues. After they are fixed, I suggest taking a quick look to verify that the "compile:examples" is run as expected in CI before merging.

[vmarchaud]

friendly ping @andyfleming if you find the time to wrap this one :)

[andyfleming]

I'm having trouble reproducing the CI error locally. Here's the CI error. It doesn't look like an error directly in the example files. Any hunch on what's going wrong?

> [email protected] compile:examples
> lerna run compile:examples

lerna notice cli v3.22.1
lerna info versioning independent
lerna info ci enabled
lerna info Executing command in 1 package: "npm run compile:examples"
lerna ERR! npm run compile:examples exited 2 in '@opentelemetry/instrumentation-express'
lerna ERR! npm run compile:examples stdout:

> @opentelemetry/[email protected] compile:examples
> cd examples && npm run compile


> [email protected] compile
> tsc -p .

node_modules/@opentelemetry/sdk-trace-base/build/src/BasicTracerProvider.d.ts:1:29 - error TS2614: Module '"@opentelemetry/api"' has no exported member 'TracerOptions'. Did you mean to use 'import TracerOptions from "@opentelemetry/api"' instead?

1 import { TextMapPropagator, TracerOptions, TracerProvider } from '@opentelemetry/api';
                              ~~~~~~~~~~~~~


Found 1 error in node_modules/@opentelemetry/sdk-trace-base/build/src/BasicTracerProvider.d.ts:1


lerna ERR! npm run compile:examples exited 2 in '@opentelemetry/instrumentation-express'
Error: Process completed with exit code 2.

[Flarna]

most likely caused by a mixmax of installed @opentelemetry/api versions. TraceOptions was added in API 1.1.0 so I guess in CI 1.0.x is installed next to SDK 1.1.0.
The reason for a diff between local run and ci is maybe the different lerna bootstrap commend used (see here)

[andyfleming]

Will open-telemetry/opentelemetry-js#2892 help with that issue?

Alternatively, is there a recommended workaround? I imagine I could switch to an older version of the API that matches the one getting installed in CI.

[Flarna]

Will open-telemetry/opentelemetry-js#2892 help with that issue?

Yes I think so.

[dyladan]

Once we have a release I think the CI will be much easier.

[dyladan]

@andyfleming thank you for your patience. Release PR is prepared and should merge/release today I hope

[andyfleming]

Thanks for following up on this @rauno56 ! 🙂

[rauno56]

Sorry for this taking so long! Thanks and congrats for your first contribution!