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

bug: filter out variable data from URI metric #421

Merged
merged 11 commits into from
Feb 12, 2020
Merged

bug: filter out variable data from URI metric #421

merged 11 commits into from
Feb 12, 2020

Conversation

jrconlin
Copy link
Member

@jrconlin jrconlin commented Feb 5, 2020

Description

filter out UID and batch numbers from URI.

This drops the first two path elements from the URI as well as masks any large hexidecimal identifier that may appear in the URI parameters. This should retain enough diagnostic information to be useful without causing overload for sentry.

Testing

While running normal testing note that the uri.path metric goes from something like:
/1.5/135772380/storage/history?batch=5ef59eae10244edc90935bab2ffea0f0&commit=true
to
storage/history?batch=###&commit=true

Issue(s)

Closes #420

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Closes #420
@jrconlin jrconlin requested a review from a team February 5, 2020 17:47
src/web/middleware/sentry.rs Outdated Show resolved Hide resolved
src/web/middleware/sentry.rs Outdated Show resolved Hide resolved
src/web/middleware/sentry.rs Outdated Show resolved Hide resolved
@jrconlin jrconlin requested a review from fzzzy February 5, 2020 21:03
fzzzy
fzzzy previously approved these changes Feb 6, 2020
Copy link
Contributor

@fzzzy fzzzy left a comment

Choose a reason for hiding this comment

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

👍

@pjenvey
Copy link
Member

pjenvey commented Feb 6, 2020

I don't think it's terribly important for uri.path to be a sentry tag. It could be sent as an "extra" instead, which isn't indexed.

A similar argument could probably be made for other tags we currently send.

If we want to keep it as a tag we'd probably want to further parse the query parameters so they're sorted (so ?batch=#&commit=true aren't differentiated from ?commit=true&batch=#)

@jrconlin jrconlin requested review from fzzzy and a team February 7, 2020 23:23
@@ -95,8 +94,6 @@ where
tags.tags.insert(k, v);
}
};
Copy link
Member

Choose a reason for hiding this comment

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

let's continue sending it to Sentry but in its extras for now

Copy link
Member Author

Choose a reason for hiding this comment

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

Sadly, the lib we're using doesn't understand how to send "extras", only "tags".

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I might be confusing this with the cadence library. Sorry. That kind of morning.

pjenvey
pjenvey previously approved these changes Feb 12, 2020
Copy link
Member

@pjenvey pjenvey left a comment

Choose a reason for hiding this comment

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

r+ pending the clippy warning (not sure what happened there).

@jrconlin jrconlin merged commit 3986c45 into master Feb 12, 2020
@pjenvey pjenvey deleted the bug/420 branch February 12, 2020 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't track uri path as Sentry tag.
3 participants