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

fix(pii): Fix issue where $span would not be recognized in Advanced Data Scrubbing #781

Merged
merged 6 commits into from
Sep 21, 2020
Merged

fix(pii): Fix issue where $span would not be recognized in Advanced Data Scrubbing #781

merged 6 commits into from
Sep 21, 2020

Conversation

untitaker
Copy link
Member

No description provided.

@untitaker untitaker requested a review from a team September 19, 2020 16:37
@untitaker untitaker changed the title fix(pii): Allow people to actually write $span fix(pii): Fix issue where $span would not be recognized in Advanced Data Scrubbing Sep 19, 2020
@untitaker untitaker changed the title fix(pii): Fix issue where $span would not be recognized in Advanced Data Scrubbing fix(pii): Fix issue where $span would not be recognized in Advanced Data Scrubbing Sep 19, 2020
jan-auer
jan-auer approved these changes Sep 20, 2020
View reviewed changes
Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

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

The macro will potentially obstruct missing variant errors, but is a clear improvement over what we had before.

relay-general/src/macros.rs Outdated Show resolved Hide resolved
@untitaker @jan-auer
Update relay-general/src/macros.rs
bd56396 
Co-authored-by: Jan Michael Auer <mail@jauer.org>
@untitaker
Copy link
Member Author

potentially obstruct missing variant errors

It will fail compilation for sure but I have no idea if the error message is pretty

@untitaker
Copy link
Member Author

ftr, as per slack suggestion I looked into strum and found it harder to navigate wrt what values get used for Display and which values are allowed. This is from the wiki for Display:

Of the various serialize properties, the value with the longest length is chosen. If that behavior isn't desired, you should use to_string.

https://github.com/Peternator7/strum/wiki/Derive-Display

That's not what we want, so we'd need to set to_string for ValueType all the time.

@jan-auer
Copy link
Member

What if we merge this in and look into strum separately? What you have now is undoubtedly less error-prone.

@untitaker
Copy link
Member Author

blocked on #780

@untitaker untitaker merged commit d69e21a into master Sep 21, 2020
@untitaker untitaker deleted the fix/add-span-fromstr branch September 21, 2020 09:34
jan-auer added a commit that referenced this pull request Sep 21, 2020
@jan-auer
Merge branch 'master' into feat/redirect-dsn
b3d44a1 
* master:
  feat: Scrubbing of UTF-16 strings in minidumps (#742)
  meta: Update CI badges (#782)
  fix(pii): Fix issue where `$span` would not be recognized in Advanced Data Scrubbing (#781)
  ci: Port CI to GitHub Actions (#780)
  fix(setup): Log when reporting to Sentry is disabled (#779)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jan-auer jan-auer jan-auer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@untitaker @jan-auer