Skip to content

Don't include headers & request info in tracing span or breadcrumb#1199

Merged
st0012 merged 2 commits intomasterfrom
reduce-unnecessary-payload
Jan 14, 2021
Merged

Don't include headers & request info in tracing span or breadcrumb#1199
st0012 merged 2 commits intomasterfrom
reduce-unnecessary-payload

Conversation

@st0012
Copy link
Copy Markdown
Contributor

@st0012 st0012 commented Jan 13, 2021

Data like request or headers can be huge in some applications. They're also likely to contain complex data structure that's hard/impossible to serialize (some might cause infinite loop).

Consider those information should have been collected in the request interface of the event, storing them in breadcrumbs could cause more harm than benefit.

@st0012 st0012 added this to the 4.1.4 milestone Jan 13, 2021
@st0012 st0012 self-assigned this Jan 13, 2021
@codecov-io
Copy link
Copy Markdown

codecov-io commented Jan 13, 2021

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.90%. Comparing base (f701932) to head (8acb142).
⚠️ Report is 1124 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1199      +/-   ##
==========================================
+ Coverage   97.99%   98.90%   +0.91%     
==========================================
  Files         192       28     -164     
  Lines        8181      733    -7448     
==========================================
- Hits         8017      725    -7292     
+ Misses        164        8     -156     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@st0012 st0012 added the Traces label Jan 13, 2021
Data like `request` or `headers` can be huge in some applications.
They're also likely to contain complex data structure that's
hard/impossible to serialize (some might cause infinite loop).

Consider those information should have been collected in the `request`
interface of the event, storing them in breadcrumbs could cause more
harm than benefit.
@st0012 st0012 changed the title Don't include headers & request info in tracing span Don't include headers & request info in tracing span or breadcrumb Jan 13, 2021
@st0012 st0012 merged commit 7be2f1d into master Jan 14, 2021
@st0012 st0012 deleted the reduce-unnecessary-payload branch January 14, 2021 01:56
@leonid-shevtsov
Copy link
Copy Markdown

This is not enough, you need to also remove response - in particular because it contains a reference to request - and many other nasty objects too.

st0012 added a commit that referenced this pull request Jun 3, 2021
@st0012
Copy link
Copy Markdown
Contributor Author

st0012 commented Jun 3, 2021

@leonid-shevtsov I'm sorry that I missed this comment 🙇 I've added #1463 to fix the issue.

st0012 added a commit that referenced this pull request Jun 3, 2021
st0012 added a commit that referenced this pull request Jun 3, 2021
* Correct breadcrumb logger's test setup

* Make sure span/breadcrumb doesn't contain response data

For the same reason mentioned in #1199

* Extract data cleanup logic to a helper module

* Update changelog
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.

3 participants