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

Conversation

@AbhiPrasad
Copy link
Member

@AbhiPrasad AbhiPrasad commented Jan 18, 2023

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

vercel bot commented Jan 18, 2023

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 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
Contributor

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
Member

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.

Copy link
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
Member Author

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.

@antonpirker
Copy link
Contributor

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants