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

✨ feat(threads): Update threading behavior for Uptime Alerts #83774

Conversation

iamrajjoshi
Copy link
Member

this PR fixes threads for Uptime Issues. Instead of creating 1 thread per issue, we will create 1 thread per open period of an issue, which in this case would be 1 thread per downtime.

this is tested via #83689 and #83601

i m having trouble verifying this locally so still verifying, but if it takes too much time, i will FF and testin an org

depends on #83689

spec: notion.so/sentry/Slack-Threads-Refactor-1618b10e4b5d807db67ae6d4d85247b9
contributes to: getsentry.atlassian.net/browse/ACI-89

ryan953 and others added 30 commits January 17, 2025 16:13
Test Notes:

This modal is behind Settings Search, the cmd+K search, and Help Search
in the sidebar.

Search works as expected with one special case I discovered: Suggestions
may include the `configUrl` field. In this case clicking on the
suggestion will trigger that url to load and be rendered as a 2nd-level
search list. To see this in action, go to Org Settings and search for
something like "Github". One of the results will be the GitHub
Integration, where you can connect to a github org. Clicking on that
result will open the 2nd screen with a list of available github orgs to
choose.

| First search | list from `configUrl` |
| --- | --- |
| <img width="820" alt="SCR-20250117-iogq"
src="https://github.com/user-attachments/assets/4b65213a-f90e-427b-96c7-e5387b992de3"
/> | <img width="696" alt="SCR-20250117-iolm"
src="https://github.com/user-attachments/assets/a6b9161c-b7db-4643-b67b-5795b6cd9c8d"
/>


Also before, if there was nothing returned from the `configUrl` url,
only an empty array for example, we'd see an empty modal:
<img width="830" alt="SCR-20250117-jdyq"
src="https://github.com/user-attachments/assets/ed8003f5-0099-48d7-aea8-1e59f3115293"
/>

This PR adds a call to `sharedProps.onFinish(sharedProps.nextPath);`
which will redirect the user to the root of the config page instead of
showing the blank modal. In my testing the root is something like
`/settings/integrations/github/`.
When the modal is not blank, and there is a list of 2nd-level things to
pick from, the url would've been constructed as `
`"/settings/integrations/github/" + pickedItem.value + "/"`. So
redirecting just into the root is a little bit of a guess and might
break in some cases.

---------

Co-authored-by: Scott Cooper <[email protected]>
Co-authored-by: getsantry[bot] <66042841+getsantry[bot]@users.noreply.github.com>
…83693)

Looking at our biome config here we can see all the linty rules that are
enabled for JavaScript files:
https://github.com/getsentry/sentry/blob/fc9ccb0f4754939a4cf19bfdb08b526f5a491f5a/biome.json#L14-L87

The docs provide the name of the equiv rule inside eslint core, or
inside other eslint plugins if available. I mapped the names of the
biome rules to those eslint rules here (below).

Then I went through each rule and enabled it within the codebase. It was
meant to be trivial because biome should've prevented violations, but a
few places did need to be fixed up. Anywhere that had a biome-ignore
statement needed an eslint one too.

There are still a few rules that biome implements which are not in our
eslint config right now. The categories are broadly:
1. Rules from plugins that i haven't installed yet, like from
`unicorn/*` or from `barrel-files/*`. I'll import these plugins in a
followup and check things off the list.
2. Some rules have no eslint equiv: `noApproximativeNumericConstant` &
`noMisrefactoredShorthandAssign`, they're also not important to have
enabled imo
3. Some rules have an eslint equiv but are also not important:
`noShoutyConstants` & `noInvalidUseBeforeDeclaration` would cause lots
of churn in the repo for example. Others are repalced by prettier.
4. Some rules depend on type annotations, these are challenging because
they are often nice to have but type annotations are _slow_ in eslint.
It would make pre-commit checks really terrible if that was enabled.
`consistent-type-imports` is an example.

I intend to get plugins to fill these specific biome gaps while
maintaining pre-commit perf.

----

The rules table is here:

| Biome Rule | ESLint Rule | Handled by |
| ------------------------------------- |
------------------------------------------------- |
------------------------ |
| noBlankTarget | react/jsx-no-target-blank | ✅ react/recommended
| noGlobalObjectCalls | no-obj-calls | ✅ tsc
| noUnreachable | no-unreachable | ✅ tsc
| useIsNan | use-isnan | ✅ eslint/recommended
| noUnusedPrivateClassMembers | no-unused-private-class-members | ✅
eslint/recommended
| noInvalidUseBeforeDeclaration |
@typescript-eslint/no-use-before-define | ✅ Disabled in sentry
| noNodejsModules | import/no-nodejs-modules | ✅ eslint.config (except
scripts)
| useFlatMap | unicorn/prefer-array-flat-map | TODO
| useOptionalChain | @typescript-eslint/prefer-optional-chain | TODO
typescript/stylistic-type-checked
| noEmptyTypeParameters | Prettier? | TODO
| noUselessLoneBlockStatements | no-lone-blocks | ✅ eslint.config
| noUselessEmptyExport | @typescript-eslint/no-useless-empty-export | ✅
eslint.config
| noUselessConstructor | @typescript-eslint/no-useless-constructor | ✅
typescript/strict
| noUselessTypeConstraint |
@typescript-eslint/no-unnecessary-type-constraint | ✅
typescript/recommended
| noExcessiveNestedTestSuites | jest/max-nested-describe (max=5) | ✅
eslint.config
| noBarrelFile | barrel-files/avoid-barrel-files | TODO
| noDangerouslySetInnerHtmlWithChildren | react/no-danger-with-children
| ✅ react/recommended
| noDebugger | no-debugger | ✅ eslint/recommended
| noDoubleEquals | eqeqeq | ✅ eslint.config
| noDuplicateJsxProps | react/jsx-no-duplicate-props | ✅
react/recommended
| noDuplicateObjectKeys | no-dupe-keys | ✅ tsc
| noDuplicateParameters | no-dupe-args | ✅ tsc
| noDuplicateCase | no-duplicate-case | ✅ eslint/recommended
| noFallthroughSwitchClause | no-fallthrough | ✅ eslint/recommended
| noRedeclare | @typescript-eslint/no-redeclare | ✅ tsc
| noSparseArray | no-sparse-arrays | ✅ eslint recommended
| noUnsafeDeclarationMerging |
@typescript-eslint/no-unsafe-declaration-merging | ✅
typescript/recommended
| noUnsafeNegation | no-unsafe-negation | ✅ tsc
| useIsArray | unicorn/no-instanceof-array | TODO
| noApproximativeNumericConstant | approx_constant | TODO
| noMisrefactoredShorthandAssign | misrefactored_assign_op | TODO
| useAwait | require-await & @typescript-eslint/require-await | ✅
esint.config
| useNamespaceKeyword | @typescript-eslint/prefer-namespace-keyword | ✅
typescript/recommended
| noFocusedTests | jest/no-focused-tests | ✅ jest/recommended
| noDuplicateTestHooks | jest/no-duplicate-hooks | ✅ eslint.config
| noCommaOperator | no-sequences | ✅ eslint.config
| noShoutyConstants | ??? | TODO
| noParameterProperties | @typescript-eslint/parameter-properties | ✅
typescript
| noVar | no-var | ✅ eslint.config
| useConst | prefer-const | ✅ tsc
| useShorthandFunctionType | @typescript-eslint/prefer-function-type | ✅
typescript/stylistic
| useExportType | @typescript-eslint/consistent-type-exports | TODO
requires types
| useImportType | @typescript-eslint/consistent-type-imports | TODO
requires types
| useNodejsImportProtocol | unicorn/prefer-node-protocol | TODO
| useLiteralEnumMembers | @typescript-eslint/prefer-literal-enum-member
| ✅ typescript/strict
| useEnumInitializers | @typescript-eslint/prefer-enum-initializers | ✅
| useAsConstAssertion | @typescript-eslint/prefer-as-const | ✅
typescript/recommended
| useBlockStatements | curly | ✅ prettier

New plugins: 
-
https://github.com/sindresorhus/eslint-plugin-unicorn/blob/main/docs/rules/prefer-array-flat-map.md
-
https://github.com/thepassle/eslint-plugin-barrel-files/blob/main/docs/rules/avoid-barrel-files.md
- https://tanstack.com/query/v5/docs/eslint/eslint-plugin-query
The PR that originally introduced the logic for skipping builtin Node
frames was #49174. Eventually we
added Windows paths (`X:\…`). We have now seen customer events with
`abs_path`s starting with `http(s)`, so we consider those non-builtin as
well.
…onboarding docs of 'unity', 'minidump' and 'unreal' (#83642)
Adds `confidence` as a widget viewer context prop. When the data is
fetched in `spanWidgetQueries` we call `onDataFetched` with the
confidence. I updated the `onDataFetched` to overwrite keys instead of
the whole object so I can add the new `confidence` key.

That's set inside `data` in the widget card. Then, when the expand icon
gets clicked, that data gets passed down to a call that sets the widget
viewer context. That widget viewer context is used in `detail.tsx` to
supply a `confidence` value to the `openWidgetViewerModal` function
Sorting isn't necessary when you don't have a grouping, so update the
state to set a default when a grouping is added (with priority for the
yAxis if it exists) and clear the sort if a yAxis changes and a grouping
is not applied.
Moving some code around, in preparation to integrate standards widgets.

- Move error handling higher up. Instead of having a bunch of error
panels everywhere, check once at the top of the `render` method
- Move series colorization higher in the component. Moves the code up to
reduce nesting, and makes it easier to return early if needed
- Resolve duplication of JSX. Instead of a conditional, pass `enabled`
to `ReleaseQueries` to prevent unnecessarily loading data
…rs (#83633)

Previously, we only had one node lambda layer called
`SentryNodeServerlessSDK`. With the `v8` major of the sentry-javascript
SDKs a new layer for `v7` was published under
`SentryNodeServerlessSDKv7` while the `v8` layer rolled over into
`SentryNodeServerlessSDK`.

With `v8` the `@sentry/serverless` package was removed (see: #70137) and
a version check was added to the integration to update `NODE_OPTIONS`
for the integration accordingly. The version check assumes the
`SentryNodeServerlessSDK` name and only checks the ARN version which is
not enough to identify a `v7` layer since the introduction of
`SentryNodeServerlessSDKv7`.

With `v9` we will stop updating the `SentryNodeServerlessSDK` and
instead publish `SentryNodeServerlessSDKv9` exclusively, the integration
has to ensure the correct `NODE_OPTIONS` are set depending on layer name
and ARN version which this PR adds.

Closes: #82646
…83667)

This reverts commit fae6fd3 which got
reverted in #83640.

It was reverted because I made the mistake of removing the option before
this was fully out.
In #83475 we started passing the `event_id` instead of the Node data to
prevent passing pickled data.

Unfortunately, that causes an extra Snuba call (which can fail) to
determine the group ID.

If we pass the group_id when scheduling the task we won't need to call
Snuba.

Fixes [SENTRY-3MGG](https://sentry.sentry.io/issues/6222497528/)
…#83681)

There was a request to have a tooltip displaying all teams that have
access to a dashboard in [this slack
thread](https://sentry.slack.com/archives/C072KUA8R1U/p1736457766301069).
Now hovering over the +(a number) bubble in the avatar list will give a
tooltip of all the teams (some of them are scrollable due to the large
amount of teams).

Here's what it looks like:
<img width="778" alt="image"
src="https://github.com/user-attachments/assets/40086f9c-9562-40a6-ab11-a1195ebf6471"
/>
When rendering series names that contain newline characters, make sure
we format them nicely by replacing them with a single white space.

Also lifts the fix for escaped character names in the legend into the
legend function directly instead of using an override in the insights
chart component that loses the truncation formatter.
Recently, I've changed the header height for the trace view to better
match the designs, but forgot to update the height for the trace view
preview on Issue Details. This will stop the bottom of the preview from
getting cut off early
Wraps the menu item for "Add to Dashboard" with a feature component to
hook into for upselling. The implementation is a little wonky here
because we don't typically flag menu items like this, but we need it to
disable the Add to Dashboard button when a user clicks it. If the user
doesn't have `dashboards-edit` access, we should disable the item. If
they don't have `dashboards-eap` access, then we shouldn't show the menu
item at all.

Ideally, if I went to see this on a team plan I would see the popover
telling me I need the business plan when I hover.

![Screenshot 2025-01-20 at 2 08
19 PM](https://github.com/user-attachments/assets/c91b9366-eb8c-479c-9593-7a8c089d8e33)
Before: 
<img width="788" alt="Screenshot 2025-01-20 at 2 59 11 PM"
src="https://github.com/user-attachments/assets/bdf9a2a4-e17e-4984-96e7-c4ac37400267"
/>

After:
<img width="897" alt="Screenshot 2025-01-20 at 2 58 50 PM"
src="https://github.com/user-attachments/assets/c8e8cad1-5b55-4db8-8fb1-c26f44470f81"
/>

Co-authored-by: Abdullah Khan <[email protected]>
…ew (#83699)

Right now, clicking on a span ID in the panel keeps the panel open, and
navigates to the trace view. Closing the panel then calls the `onClose`
callback, and sometimes clobbers the URL.

Navigating to the sample view should close the panel, this PR adds that
check. This is a hotfix, the proper solution is to make the navigation
behaviour more robust, probably by rendering the panel view at a
`/samples/` sub-route, which is easier to detect on navigation.
This PR updates the relay library and removes default cardinality limits
for profiling metrics, which relay does not interpret anymore.
Uses the `validateWidgetQuery` hook to get widget validation
issues/warnings for ondemand. This seems to apply to the group by field
and filter fields.
Use `sentry-opentelemetry-agent` for Java SDK v8 onboarding.

### Legal Boilerplate

Look, I get it. The entity doing business as "Sentry" was incorporated
in the State of Delaware in 2015 as Functional Software, Inc. and is
gonna need some rights from me in order to utilize my contributions in
this here PR. So here's the deal: I retain all rights, title and
interest in and to my contributions, and by keeping this boilerplate
intact I confirm that Sentry can use, modify, copy, and redistribute my
contributions, under Sentry's choice of terms.
…s selected a second time (#83726)

Right now the top stats for mobile insights (e.g. Avg Cold Start R1)
remain empty, unless a release is picked a second time:


https://github.com/user-attachments/assets/088e2945-c3b3-4d7a-8ecf-36b48aa417cb
<!-- Describe your PR here. -->
scttcper and others added 13 commits January 21, 2025 19:07
https://github.com/getsentry/status-page-list/blob/main/CHANGELOG.md

Adds entries for Jotform, Paubox, SendGrid, Autodesk, Etsy, InfluxData,
Loom, Miro, Monday.com, PlanetScale, Pleo, Ravelin, Render, Rippling,
Squarespace, Twilio, and Vanta.

Improves entries for Atlassian and Vercel.
There were suggestions from the bug bash to change the default display
type for widgets in the widget builder to be line charts. Now all
datasets except for Issues will default to line charts.
#83763)

Aggregates that don't have an argument (e.g. `count()`) don't require
you to select a column so we're going to not display the column
selector. This enables the user so they can start selecting different
values instead of seeing a disabled state.
We attempted this in #80749

However due to some overly agressive timeout restrictions this failed in
US but applied in DE, leaving us in a weird un-deployable state.

We removed the constraint here
#80876

But we do need this constraint, otherwise we're not able to toggle
'trace sampling'
In the last month we saw 85% of new feedback envelopes filtered because
they were missing a feedback context or message. 2% filtered because
message was present, but empty. These logs will help us get some info on
the specific orgs/projects this is happening for.
Supports cron monitors in the alerts list
The "Every" was already part of the interval text
Explore will ship with RPC enabled by default, so enable dashboards with
it as well
@iamrajjoshi iamrajjoshi requested a review from a team January 21, 2025 21:21
@iamrajjoshi iamrajjoshi self-assigned this Jan 21, 2025
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jan 21, 2025
):
open_period_start = open_period_start_for_group(event.group)
# Save in the notification message object so it can be used in the repository
new_notification_message_object.open_period_start = open_period_start
Copy link
Member Author

Choose a reason for hiding this comment

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

this will be saved via

self._repository.create_notification_message(
data=new_notification_message_object
)

Copy link

codecov bot commented Jan 21, 2025

Codecov Report

Attention: Patch coverage is 85.56604% with 153 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
static/app/bootstrap/index.tsx 0.00% 11 Missing ⚠️
...nts/events/interfaces/spans/spanProfileDetails.tsx 0.00% 11 Missing ⚠️
...newTraceDetails/traceDrawer/details/span/index.tsx 38.46% 8 Missing ⚠️
static/app/components/charts/utils.tsx 0.00% 7 Missing ⚠️
src/sentry/api/serializers/models/group_stream.py 94.11% 6 Missing ⚠️
src/sentry/feedback/usecases/create_feedback.py 44.44% 5 Missing ⚠️
...ding/gettingStartedDoc/storeCrashReportsConfig.tsx 64.28% 5 Missing ⚠️
static/app/views/dashboards/widgetCard/chart.tsx 72.22% 2 Missing and 3 partials ⚠️
...p/views/insights/common/utils/useSamplesDrawer.tsx 0.00% 5 Missing ⚠️
...tatic/app/views/settings/project/tempest/index.tsx 0.00% 5 Missing ⚠️
... and 44 more
Additional details and impacted files
@@                         Coverage Diff                          @@
##           raj/slack-ref/latest-open-period   #83774      +/-   ##
====================================================================
- Coverage                             87.57%   85.17%   -2.40%     
====================================================================
  Files                                  9488     9506      +18     
  Lines                                539335   541991    +2656     
  Branches                              21230    21210      -20     
====================================================================
- Hits                                 472338   461667   -10671     
- Misses                                66649    79870   +13221     
- Partials                                348      454     +106     

@iamrajjoshi iamrajjoshi force-pushed the raj/slack-ref/fix-uptime-threads branch from 3eb022d to 5cc8447 Compare January 21, 2025 22:07
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Jan 21, 2025
Copy link
Contributor

🚨 Warning: This pull request contains Frontend and Backend changes!

It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently.

Have questions? Please ask in the #discuss-dev-infra channel.

Copy link

codecov bot commented Jan 21, 2025

Bundle Report

Changes will increase total bundle size by 1.06MB (3.39%) ⬆️. This is within the configured threshold ✅

Detailed changes
Bundle name Size Change
app-webpack-bundle-array-push 32.35MB 1.06MB (3.39%) ⬆️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Backend Automatically applied to PRs that change backend components Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.