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

feat(node): Add genericPool integration using opentelemetry instrumentation #13356

Closed

Conversation

Zen-cronic
Copy link
Contributor

For #13308

Implement genericPool OTL instrumentation in packages/node

Todo:

  • If you've added code that should be tested, please add tests.
  • Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

Zen-cronic and others added 3 commits August 13, 2024 10:47
…tation

Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>
…tation

Set up initial code and include this instrumentation as a default integration.

Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>
@lforst lforst self-assigned this Aug 14, 2024
@Zen-cronic
Copy link
Contributor Author

hey @lforst , thanks for assigning to this pr.

This is how I'm currently testing the updated @sentry/node package in node-integration-tests:

  1. yarn build (in root)
  2. yarn yalc:publish (in root)
  3. cd dev-packages/node-integration-tests && yalc add @sentry/node @sentry/utils @sentry/types @sentry/core (according to this guide)

The changes are observed (so it's working!), but yalc generates a .yalc dir and yalc.lock in node-integration-tests that contains the linked packages. But it doesn't seem like the right approach (we can't push those yalc-related to CI).

Is yarn link a more suitable approach (but this also generates yalc.lock)?

nicohrubec and others added 23 commits August 26, 2024 13:37
…ted (getsentry#13278)

[ref](getsentry#12351)

This PR moves the `SentryGlobalFilter` out of the root module, which led
to the filter overriding user exception filters in certain scenarios.
Now there is two ways to setup sentry error monitoring:
- If users have no catch-all exception filter in their application they
add the `SentryGlobalFilter` as a provider in their main module.
- If users have a catch-all exception filter (annotated with `@Catch()`
they can use the newly introduced `SentryCaptureException()` decorator
to capture alle exceptions caught by this filter.

Testing:
Added a new sample application to test the second setup option and
expanded the test suite in general.

Side note:
- Also removed the `@sentry/microservices` dependency again, since it
does not come out of the box with every nest application so we cannot
rely on it.
This streamlines some caching stuff for CI:

1. Extract dependency installation & cache out into a composite action
for reusability
2. Updated the cache key for dependencies to only include package &
dev-package `package.json`, not the E2E test ones.
Instead of having to keep to separate lists of include/excludes, we now
keep this in a single list and invert it when necessary.

This way, we should no longer have problems where tests are either run
multiple times, or not in the correct env - just add the test to the
`browser` list in `ci-unit-tests.ts` to make sure it is not run in
multiple node versions unnecessarily. I also added this step to the new
package checklist.
This improves a few things in our size-limit CI action:

1. Show change in bytes, in addition to the change in percentage. 
2. Add a link below the table to the base comparison run.
3. If we detect that the workflow run we used as base is not the latest
one, show a warning on top.


![image](https://github.com/user-attachments/assets/4678ff04-a463-4579-ad91-74cbf9b7d781)
…etsentry#13338)

This streamlines some stuff in our browser integration tests, to fix
some flakiness (hopefully).

The biggest change is that instead of always building into `dist` for
each test file, each test will now build into a random subfolder, e.g.
`dist/abc`. This way, multiple tests in a single file will never
conflict with each other.

Additionally it also streamlines some of the tests I encountered while
looking at stuff, hopefully reducing flakes further.

Closes getsentry#13321
Reverts getsentry#13118

There has been quite a bit of segmentation faults in CI after this PR
was merged.

I tried spending time to look into this in more detail, but it didn't
get far, so not worrying about this too much for now. Let's revert and
re-examine later.
relax the pageload transaction assertion. Web vitals are too
flaky to hard-assert on them and they are covered in browser integration
tests
Minor changes: I realized that the formatting was broken for `v8` and
that the migration guide wasn't linking to the specific section like
other changelogs.
CI is angery. Crypto is only on global from Node 19 onwards.
This PR adds the external contributor to the CHANGELOG.md file, so that
they are credited for their contribution. See getsentry#13395

Co-authored-by: mydea <2411343+mydea@users.noreply.github.com>
If the domain is not in `Allowed Domains` in the Sentry project
settings, it would cause a 403 error. The default setting is `*` so this
only occurs when the user changes these settings.

Fixes getsentry#9856
…tation

Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>
…tation

Set up initial code and include this instrumentation as a default integration.

Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>
Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>
@Zen-cronic Zen-cronic closed this Aug 26, 2024
@Zen-cronic
Copy link
Contributor Author

Apologies for the mess. Please check out the new PR at #13465

@lforst lforst removed their assignment Aug 28, 2024
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.