Skip to content

ref(node): Refactor node source fetching into integration - #3729

Merged
AbhiPrasad merged 45 commits into
getsentry:masterfrom
timfish:feat/separate-source-reading
Feb 23, 2022
Merged

ref(node): Refactor node source fetching into integration#3729
AbhiPrasad merged 45 commits into
getsentry:masterfrom
timfish:feat/separate-source-reading

Conversation

@timfish

@timfishtimfish commented Jun 22, 2021•
edited
Loading

Copy link
Copy Markdown
Collaborator

This PR:

  • Moves eventFromException and eventFromMessage into eventbuilder to match browser SDK
  • Moves source context loading/addition into a ContextLines integration
  • frameContextLines option gets copied to ContextLines constructor but also pulls frameContextLines from NodeOptions to avoid a breaking change.
  • frameContextLines in NodeOptions marked as @deprecated so users learn about future migration

@kamilogorek

kamilogorek commented Jun 22, 2021

Copy link
Copy Markdown
Contributor

I like this approach much better. One thing that we might try though is to go even further and move context lines to a separate integration and add it by default. I already implemented it in v7-dev branch so you can basically copy-paste (kinda) my code from here: https://github.com/getsentry/sentry-javascript/blob/v7-dev/packages/integration-node-contextlines/src/index.ts

@timfishtimfish mentioned this pull request Jun 23, 2021
Closed
@timfish

This comment has been minimized.

Sign in to view
@kamilogorek

kamilogorek commented Jun 25, 2021

Copy link
Copy Markdown
Contributor

That's definitely a valid concern, although with a 100 items cache it should be a very minor if any performance issue. wdyt @rhcarvalho@HazAT

@timfish

This comment has been minimized.

Sign in to view
@timfishtimfish changed the title Feat: Refactor node source fetching into separate step to stack parsingFeat: Refactor node source fetching into integrationJul 20, 2021
@timfish

This comment has been minimized.

Sign in to view
timfish
timfish commented Jul 20, 2021
View reviewed changes
Comment threadpackages/node/test/index.test.ts Outdated
@timfish

This comment has been minimized.

Sign in to view
@kamilogorekkamilogorek changed the title Feat: Refactor node source fetching into integrationref(node): Refactor node source fetching into integrationSep 14, 2021
@github-actions

github-actionsBot commented Oct 14, 2021

Copy link
Copy Markdown
Contributor

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@timfish

timfish commented Oct 14, 2021

Copy link
Copy Markdown
CollaboratorAuthor

I promise, I do intend to finish this when I'm back from my extended vacation.

@timfish

timfish commented Feb 15, 2022•
edited
Loading

Copy link
Copy Markdown
CollaboratorAuthor

@lobsterkatie maybe you can advise on these nextjs test failures?

From what I can tell, it's completely expected for captureEvent to be called before the event is rejected by the event processors.

I'm not quite sure why my changes have caused this to start failing 🤔.
My best guess is that it's because this PR removes some async'iness.

@timfish
timfishforce-pushed the feat/separate-source-reading branch from 03cb320 to e08bcbbCompareFebruary 16, 2022 20:47
lobsterkatie
lobsterkatie reviewed Feb 16, 2022
View reviewed changes

@lobsterkatielobsterkatie left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to be a good change!

Comment threadpackages/node/src/sdk.ts Outdated
Comment threadpackages/node/test/parsers.test.ts Outdated
Comment threadpackages/node/test/parsers.test.ts Outdated
Comment threadpackages/node/test/parsers.test.ts Outdated
Comment threadpackages/node/src/types.ts Outdated
Comment threadpackages/node/src/integrations/contextlines.ts
Comment threadpackages/node/src/integrations/contextlines.ts Outdated
Comment threadpackages/node/src/integrations/contextlines.ts Outdated
Comment threadpackages/node/src/integrations/contextlines.ts Outdated
Comment threadpackages/node/src/integrations/contextlines.ts Outdated
@lobsterkatie

lobsterkatie commented Feb 16, 2022

Copy link
Copy Markdown
Member

@lobsterkatie maybe you can advise on these nextjs test failures?

Seems like you figured it out, but LMK if you still need help!

@timfish

timfish commented Feb 17, 2022•
edited
Loading

Copy link
Copy Markdown
CollaboratorAuthor

Yep worked it out in the end. Thanks for the code review.

This theoretically means that a load of async can be removed from the browser/node SDKs because it was only there to cater for the node code context loading!

AbhiPrasad
AbhiPrasad reviewed Feb 17, 2022
View reviewed changes
Comment threadpackages/node/src/integrations/contextlines.ts
Comment threadpackages/node/src/types.ts
/**
* Sets the number of context lines for each frame when loading a file.
*
* @deprecated Context lines configuration has moved to the `ContextLines` integration, and can be used like this:

@AbhiPrasadAbhiPrasadFeb 17, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 👍. We'll need to update docs for this as well.

Comment threadpackages/node/src/integrations/contextlines.ts
Comment threadpackages/node/src/integrations/contextlines.ts
Comment threadpackages/node/src/integrations/contextlines.ts
Comment threadpackages/node/src/integrations/contextlines.ts Outdated
@timfish
timfishforce-pushed the feat/separate-source-reading branch from 09f27da to ef7d423CompareFebruary 17, 2022 19:47
@timfish

This comment was marked as outdated.

Sign in to view
AbhiPrasad
AbhiPrasad reviewed Feb 18, 2022
View reviewed changes
Comment threadpackages/node/src/integrations/contextlines.ts Outdated
AbhiPrasad
AbhiPrasad approved these changes Feb 18, 2022
View reviewed changes

@AbhiPrasadAbhiPrasad left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can only merge once we have the docs PR ready to go

Comment threadpackages/node/src/integrations/contextlines.ts
@timfishtimfish mentioned this pull request Feb 18, 2022
Merged
@timfish

timfish commented Feb 18, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Docs PR done 🎉🎉🎉

@timfishtimfish mentioned this pull request Feb 21, 2022
Merged
@AbhiPrasad
AbhiPrasad enabled auto-merge (squash) February 23, 2022 12:09
@AbhiPrasad
AbhiPrasad merged commit c60556f into getsentry:masterFeb 23, 2022
@AbhiPrasadAbhiPrasad added this to the Pre 7.0.0 Work milestone Feb 23, 2022
@timfish
timfish deleted the feat/separate-source-reading branch February 24, 2022 12:29
@AbhiPrasadAbhiPrasad mentioned this pull request Apr 7, 2022
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lobsterkatielobsterkatielobsterkatie left review comments
+1 more reviewer
@AbhiPrasadAbhiPrasadAbhiPrasad approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

Pre 7.0.0 Work

Development

Successfully merging this pull request may close these issues.

4 participants

@timfish@kamilogorek@lobsterkatie@AbhiPrasad