Skip to content
This repository was archived by the owner on Aug 14, 2024. It is now read-only.

feat(unified-api): Define includeLocalVariables option#813

Merged
AbhiPrasad merged 1 commit intomasterfrom
abhi-local-variables
Jan 19, 2023
Merged

feat(unified-api): Define includeLocalVariables option#813
AbhiPrasad merged 1 commit intomasterfrom
abhi-local-variables

Conversation

@AbhiPrasad
Copy link
Copy Markdown
Contributor

@AbhiPrasad AbhiPrasad commented Jan 18, 2023 •
edited
Loading

Currently the PHP, Python, Ruby and NodeJS SDKs support local variables, but they all use inconsistent names for the option that gates this functionality.

I propose we rename this to includeLocalVariables / include_local_variables, unify these across the SDKs and add them when we get time

We also should decide if sendDefaultPii should have an affect on this.

cc @timfish

@vercel
Copy link
Copy Markdown

vercel bot commented Jan 18, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
develop ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Jan 18, 2023 at 4:56PM (UTC)

@AbhiPrasad AbhiPrasad mentioned this pull request Jan 18, 2023
Closed
@AbhiPrasad AbhiPrasad self-assigned this Jan 18, 2023
@AbhiPrasad AbhiPrasad changed the title feat(unified-api): Define addLocalVariables option feat(unified-api): Define includeLocalVariables option Jan 19, 2023
@antonpirker
Copy link
Copy Markdown
Contributor

antonpirker commented Jan 19, 2023

I like consistent naming, so I am in.

As this is only for errors should error be in the variable name? (bikesheding alarm)

@sl0thentr0py
Copy link
Copy Markdown
Member

sl0thentr0py commented Jan 19, 2023

let's keep the PII stuff separate, I'm working on a comprehensive proposal to solve this better.
If that gets rejected, we can gate locals behind sendDefaultPII later.

sl0thentr0py
sl0thentr0py approved these changes Jan 19, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@sl0thentr0py sl0thentr0py left a comment

Choose a reason for hiding this comment

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

fine with me, but for existing implementations, we'll have to alias to existing names, deprecate them and remove them later in a major.

@AbhiPrasad
Copy link
Copy Markdown
Contributor Author

AbhiPrasad commented Jan 19, 2023

As this is only for errors should error be in the variable name

This is a good suggestion, but since I think local variables are a stacktrace concept, it should be clear to users that this only applies to errors.

@AbhiPrasad AbhiPrasad merged commit 510c53e into master Jan 19, 2023
@AbhiPrasad AbhiPrasad deleted the abhi-local-variables branch January 19, 2023 13:12
This was referenced Jan 19, 2023
Closed
Closed
Closed
@antonpirker
Copy link
Copy Markdown
Contributor

antonpirker commented Jan 19, 2023

Python issue for this: getsentry/sentry-python#1854

This was referenced Jan 19, 2023
Closed
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@antonpirker antonpirker Awaiting requested review from antonpirker

@cleptric cleptric Awaiting requested review from cleptric

1 more reviewer

@sl0thentr0py sl0thentr0py sl0thentr0py approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@AbhiPrasad AbhiPrasad

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@AbhiPrasad @antonpirker @sl0thentr0py