Skip to content

Conversation

dcramer
Copy link
Member

@dcramer dcramer commented Nov 25, 2015

This puts some basics in place to let us do production based perf assertions.

The change itself adds duplicate queries (>= 3) and total queries (>= 25) to tasks and API endpoints.

@getsentry/api @getsentry/infrastructure

Copy link
Member Author

Choose a reason for hiding this comment

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

probably should refactor this but its not a huge deal

@dcramer
Copy link
Member Author

dcramer commented Nov 25, 2015

One odd behavior this creates is our integration tests effectively nest context managers, so i.e. the store_event task ends up suggesting it executes a LOT of queries, but really its just the sum of all child tasks.

@codecov-io
Copy link

Current coverage is 82.40%

Merging #2370 into master will increase coverage by +0.08% as of 7ced1e2

@@            master   #2370   diff @@
======================================
  Files          932     934     +2
  Stmts        36641   36718    +77
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit          30163   30257    +94
  Partial          0       0       
+ Missed        6478    6461    -17

Review entire Coverage Diff as of 7ced1e2

Powered by Codecov. Updated on successful CI builds.

@dcramer
Copy link
Member Author

dcramer commented Nov 25, 2015

@mitsuhiko @tkaemming if this seems sane to one of you guys i'll go ahead and push it out

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this really need a cryptographic hash rather than hash?

Copy link
Member Author

Choose a reason for hiding this comment

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

i always forget that python maps strings to the same

@tkaemming
Copy link
Contributor

Generally +1. (I didn't try and run this or give it a super thorough review though.)

I could see an argument for keeping track of and logging the duplicate query statements as well, but that's easy enough to do later.

@tkaemming
Copy link
Contributor

Also putting this in sentry.perfutils rather than sentry.utils.performance or something seems to give it more priority than I'd expect (but that's just my opinion.)

@dcramer
Copy link
Member Author

dcramer commented Nov 26, 2015

I may just put it in sentry.debug idk

@mattrobenolt mattrobenolt force-pushed the master branch 2 times, most recently from ab608d7 to e0a5066 Compare December 9, 2015 21:47
@tkaemming tkaemming removed their assignment Mar 9, 2016
@mattrobenolt mattrobenolt force-pushed the master branch 2 times, most recently from 03928d0 to 59cc451 Compare March 27, 2016 21:25
@dcramer dcramer force-pushed the perfutils branch 2 times, most recently from 5952483 to 643bc85 Compare April 20, 2016 16:27
@dcramer dcramer changed the title Add SQL query counts to tasks Add SQL query counts to tasks and API Apr 20, 2016
@dcramer
Copy link
Member Author

dcramer commented Apr 20, 2016

Going to get this in today

@dcramer
Copy link
Member Author

dcramer commented Apr 20, 2016

I'm thinking we need a way to whitelist things, but we can sort that out later. It's likely this will pollute Sentry a bit right now, so we'll run it for a day or two and then decide how we want to deal with it.

@dcramer dcramer merged commit c71961a into master Apr 20, 2016
@dcramer dcramer deleted the perfutils branch April 20, 2016 16:51
@github-actions github-actions bot locked and limited conversation to collaborators Dec 23, 2020
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.

3 participants