-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
refs(metrics) Count/measure the number of issues on the issue page. #35422
Conversation
One thing I notices slapping together this PR: we probably should have a "static API" for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you need to do several things right now to use the 'experimental' set_measurement
api on the SDK.
- require at least
1.5.12
here - add
_experiments={"custom_measurements": True})
to the sdk init - don't start a new transaction but use the current one on the scope
transaction = Hub.current.scope.transaction
if transaction:
transaction.set_measurement('foobar', 123)
eventually we'll add top-level api helpers, but since this is experimental this is not exposed all the way yet.
Incorporated feedback of @sl0thentr0py. Now it is a good base for discussion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good from sdk side, someone from product should still review the other stuff
We started exporting SENTRY_DSN back in #32524 in order to fix errors in lib.sh not being reported. Having this variable exported can cause a lot of errors from a developers' development to show up as dev env errors. Exporting inside of lib.sh does not persist in the developers' environment.
hi @edwardgou-sentry would you mind checking this PR? |
@antonpirker was just discussing with @jas-kas and they have some examples for testing locally using Ideally a python dev workin on product would do it, and they could make sure the SDK impl makes sense for them. @k-fish would someone on your team be able to support here? With this PR as an example of setting custom measurements. There is this notion page with example of what kinds of dashboards should be possible after these start being collected |
@antonpirker you also mention some measurement which we are currently doing with another tool, can you remind me where those are? Maybe they are some other use cases we can surface, document/promote/dog food |
Hey @smeubank, by this: I think we can start adding instrumentation to those measurements as well, but those measurements won't actually get "collected" yet (ie extracted to metrics). This is also true for the measurements in this pr. There is an allowlist in relay preventing these custom measurements from being extracted at the moment. The ingest team is working on removing this allowlist and preparing to properly ingest these custom measurements, so we won't be able to collect on these measurements until then. Sorry if this is information you already knew. 😄 |
now that you write it, it sounds familiar, but can't swear that I know anything ever 😄 but then relay just throws them away right, it doesn't cuase any issues? If that is the case then I would merge any |
Yup, i believe this is correct based on my understanding too! |
@@ -345,6 +346,10 @@ def get(self, request: Request, organization) -> Response: | |||
self.add_cursor_headers(request, response, cursor_result) | |||
|
|||
# TODO(jess): add metrics that are similar to project endpoint here | |||
transaction = Hub.current.scope.transaction | |||
if transaction: | |||
transaction.set_measurement("issues.unresolved", len(cursor_result)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could be wrong here since I'm not in issues code very much, but if a user visits the For Review
tab directly via url (ie /organizations/sentry/issues/?query=is%3Aunresolved+is%3Afor_review+assigned_or_suggested%3A%5Bme%2C+none%5D&sort=inbox
), wouldn't this measurement become inaccurate? It's reporting for issues.unresolved
but the actual measurement should be for issues.for_review
.
@@ -345,6 +346,10 @@ def get(self, request: Request, organization) -> Response: | |||
self.add_cursor_headers(request, response, cursor_result) | |||
|
|||
# TODO(jess): add metrics that are similar to project endpoint here | |||
transaction = Hub.current.scope.transaction | |||
if transaction: | |||
transaction.set_measurement("issues.unresolved", len(cursor_result)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why don't we have a global sentry_sdk.set_measurement
for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commented above
eventually we'll add top-level api helpers, but since this is experimental this is not exposed all the way yet.
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you label it "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
just silently closing this. It is not that important anymore. Thanks everyone for the reviews. |
Request for comments and discussion:
This is about dogfooding the measurements API:
getsentry/sentry-python#1359
The idea is to count/measure the number of issues displayed on the "Issues" page. (One metric for "unresolved", "for review" and "ignored" respectively)
This has multiple benefits: