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.

Sign up for GitHub

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

Jump to bottom

Can't be tested with Jest #50

Open
nirsky opened this issue Feb 15, 2021 · 3 comments
Open

Can't be tested with Jest #50

nirsky opened this issue Feb 15, 2021 · 3 comments

Comments

@nirsky
Copy link

nirsky commented Feb 15, 2021

Seems like Jest magic on the require function is clashing with require-in-the-middle.
Not sure if there's a way to overcome this.
Decided to open this issue to discuss this, maybe someone can share a solution.

Related:
@astorm
Copy link
Contributor

astorm commented Feb 16, 2021

Hello @nirsky, thanks for bringing this up. To start the conversation, two questions

  1. Do you have an example of some jest code where require-in-the-middle doesn't work like you expect?
  2. Do you happen to know what, specifically, "jest magic on the require function" is and/or where in jest they're applying this magic?

@nirsky
Copy link
Author

nirsky commented Feb 28, 2021

Hey, sorry for the late reply.
I encountered this issue while writing tests for plugins I wrote for opentelemetry.
opentelemetry uses require-in-the-middle to know when a library was required, and patch it upon requiring it.

Jest is overriding the require to create an isolated environment for each test suite, mocking and JIT (jestjs/jest#7722).
I suspect those 2 collide.
In my case I had to migrate all the tests to use mocha.

@watson
Copy link
Contributor

watson commented Mar 2, 2021

You're correct regarding jest vs require-in-the-middle. Jest uses the vm core module to run tests. I've spent a lot of time to see if we could somehow be notified of calls to require from within those vm's, but so far no luck. It should be technically possible, but jest is very complicated, so I've not been able to figure out what change I need to make to jest in order for it to work. So for now I've tabled the efforts as it was taking too much time.

@blumamir blumamir mentioned this issue Apr 14, 2021
Closed
myrotvorets-team added a commit to myrotvorets/opentelemetry-plugin-knex that referenced this issue Apr 26, 2021
@myrotvorets-team
Update to opentelemetry 0.19
6ad811e 
  * Migrate to mocha - see elastic/require-in-the-middle#50
  * Refactor from Plugin into Instrumentation
@trentm trentm mentioned this issue Dec 3, 2021
Open
3 tasks
@karlbohlmark karlbohlmark mentioned this issue Apr 14, 2023
Open
@python3js python3js mentioned this issue Apr 24, 2024
Closed
@onurtemizkan onurtemizkan mentioned this issue May 20, 2024
Merged
AbhiPrasad pushed a commit to getsentry/sentry-javascript that referenced this issue Jun 13, 2024
@onurtemizkan
feat(remix): Migrate to opentelemetry-instrumentation-remix. (#12110)
1201eb2 
Ref: #11040

Migrates Remix server-side SDK to
[opentelemetry-instrumentation-remix](https://www.npmjs.com/package/opentelemetry-instrumentation-remix).

This PR also keeps the original implementation not the break the
developer experience for non-Express Remix projects.
Remix projects using Express are supported as is using the new
`autoInstrumentRemix` option.

**Usage with Express:**

```javascript
// instrument.(cjs | mjs)
const Sentry = require('@sentry/remix');

Sentry.init({
  dsn: YOUR_DSN
  // ...
  // auto instrument Remix with OpenTelemetry
  autoInstrumentRemix: true,
  // Optionally capture action formData attributes with errors.
  // This requires `sendDefaultPii` set to true as well.
  captureActionFormDataKeys: {
    file: true,
    text: true,
  },
  // To capture action formData attributes.
  sendDefaultPii: true
});
``` 

```javascript
// server.(cjs | mjs)
// import the Sentry instrumentation file before anything else.
import './instrument.cjs';
// alternatively `require('./instrument.cjs')`

// ...

const app = express();

// ...
```

**Usage with built-in Remix server:**

You need to run the Remix server with `NODE_OPTIONS=`--require(...)`
set.

```json
// package.json

// ...
  "scripts": {
    "start": "NODE_OPTIONS='--require=./instrument.server.cjs' remix-serve build"
    // or
    "start": "NODE_OPTIONS='--import=./instrument.server.mjs' remix-serve build"
  }
// ...
```

This PR _**removes**_:
- Express server adapter.
There is no need to use `wrapExpressCreateRequestHandler` anymore even
if you don't opt-in to `autoInstrumentRemix`.
`wrapExpressCreateRequestHandler` is kept exported as a no-op function.

- Built in HTTP incoming request instrumentation for both `legacy` and
`otel` implementations. Instead we mark `requestHandler` spans as the
root `http.server` spans.

When `autoInstrumentRemix` is set to `true`, this update _**replaces**_:

- Performance tracing on `action` / `loader` / `documentRequest`
functions. Leaving them to be traced by
`opentelemetry-instrumentation-remix`
- Request handler instrumentation as they are also traced by
`opentelemetry-instrumentation-remix`
- Auto-instrumentation for http as default integration for Remix SDK.


- With the new instrumentation, `pageload` span is the child of the
`loader` span. (Example:
[Trace](https://sentry-sdks.sentry.io/performance/trace/e51c640933786fd491bb428fb1eff826/?fov=0%2C436.5&node=error-cd2dc8610c5e4affb7356147daa77393&statsPeriod=7d&timestamp=1718029546))

- Legacy instrumentation keeps recording `pageload` span as the child of
the `http.server` span. (Example:
[Trace](https://sentry-sdks.sentry.io/performance/trace/01107a555eeee9b44d1f8c25ea0c45f3/?fov=0%2C818.599853515625&node=txn-0695246d026b4da792cf8f5e0a372b50&statsPeriod=7d&timestamp=1718103519))


Also:

Migrates Remix integration tests from Jest to Vitest.
- Related issues: jestjs/jest#15033
elastic/require-in-the-middle#50

Fixes Backlogged Issues:
- Fixes: #12082 -
[Code](https://github.com/getsentry/sentry-javascript/pull/12110/files#diff-754e32c1c14ac3c3c93eedeb7680548b986bc02a8b0bc63d2efc189210322acdR416-R440)
- Fixes: #9737 -
[Test](https://github.com/getsentry/sentry-javascript/pull/12110/files#diff-1484a5311699a814a39921d440f831e9babe69f8225a0de667ebd8cc63ca5accR109)
- Fixes: #12285
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@watson @astorm @nirsky