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

Use allowlist to filter ActiveSupport breadcrumbs' data #2048

Merged
merged 2 commits into from
Jun 21, 2023

Conversation

st0012
Copy link
Collaborator

@st0012 st0012 commented May 26, 2023

Since the number of potentially hard-to-serialise data is growing, we should use allowlist to filter the data.

This should properly resolve #1183

@st0012 st0012 added this to the 6.0.0 milestone May 26, 2023
@st0012 st0012 self-assigned this May 26, 2023
@st0012 st0012 modified the milestones: 6.0.0, 5.10.0 May 26, 2023
@codecov
Copy link

codecov bot commented May 26, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (75aa456) 83.23% compared to head (29854b9) 83.23%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2048   +/-   ##
=======================================
  Coverage   83.23%   83.23%           
=======================================
  Files         119      119           
  Lines        5662     5662           
=======================================
  Hits         4713     4713           
  Misses        949      949           
Impacted Files Coverage Δ
...b/sentry/rails/breadcrumb/active_support_logger.rb 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@st0012
Copy link
Collaborator Author

st0012 commented May 26, 2023

The failures are caused by latest Rails changes. I will open another PR to address it.

@st0012 st0012 force-pushed the use-allowlist-for-active-support-breadcrumbs branch from bb5ca15 to 4d47eab Compare May 30, 2023 12:00
@st0012 st0012 requested a review from sl0thentr0py May 30, 2023 12:06
@st0012
Copy link
Collaborator Author

st0012 commented Jun 7, 2023

@sl0thentr0py Once this and #2036 is merged, I'd like to release v5.10.0. WDYT?

Since the number of potentially hard-to-serialise data is growing,
we should use allowlist to filter the data.
@st0012 st0012 force-pushed the use-allowlist-for-active-support-breadcrumbs branch from 4d47eab to 29854b9 Compare June 21, 2023 10:09
@st0012
Copy link
Collaborator Author

st0012 commented Jun 21, 2023

@sl0thentr0py I've fixed conflicts in this PR, would you mind giving it a look?

@sl0thentr0py
Copy link
Member

@st0012 do you think we can remove this line now?

::JSON.parse(::JSON.generate(@data))

@st0012
Copy link
Collaborator Author

st0012 commented Jun 21, 2023

@sl0thentr0py Probably not because users could still accidentally add data that's not serialisable, which we'd want to spot early with serialized_data

@sl0thentr0py
Copy link
Member

ok let's keep it in for now, but I would actually prefer to get rid of that eventually because ideally there should be just one large JSON serialization step in client/transport. We can make it clearer in the docs/docstrings what people should pass into data.

@st0012 st0012 merged commit 5054dbe into master Jun 21, 2023
97 checks passed
@st0012 st0012 deleted the use-allowlist-for-active-support-breadcrumbs branch June 21, 2023 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RuntimeError: can't add a new key into hash during iteration in middleware
2 participants