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

util/log: allow custom crash report tags #106786

Merged
merged 1 commit into from
Jul 14, 2023

Conversation

pjtatlow
Copy link
Contributor

Today it can be difficult to trace back a sentry event to the CC cluster where
it originated, especially for serverless clusters. This change enables a new
environment variable (COCKROACH_CRASH_REPORT_TAGS), which allows
the database operator to provide additional information that will be included
in the sentry event.

Release Note: None

@pjtatlow pjtatlow requested review from a team and dhartunian and removed request for a team July 13, 2023 20:29
@blathers-crl
Copy link

blathers-crl bot commented Jul 13, 2023

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@pjtatlow pjtatlow requested a review from jeffswenson July 13, 2023 20:29
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@jeffswenson jeffswenson left a comment

Choose a reason for hiding this comment

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

LGTM 💯

c-deps/libedit Outdated Show resolved Hide resolved
pkg/util/log/logcrash/crash_reporting.go Outdated Show resolved Hide resolved
@pjtatlow pjtatlow force-pushed the support-custom-crash-report-tags branch from 7ba6159 to 31f3f5e Compare July 13, 2023 20:56
Today it can be difficult to trace back a sentry event to the CC cluster where
it originated, especially for serverless clusters. This change enables a new
environment variable (COCKROACH_CRASH_REPORT_TAGS), which allows
the database operator to provide additional information that will be included
in the sentry event.

Release note: None

Epic: none
@pjtatlow pjtatlow force-pushed the support-custom-crash-report-tags branch from 31f3f5e to 68a82d5 Compare July 13, 2023 21:09
@pjtatlow
Copy link
Contributor Author

TFTR!

bors r+

@craig
Copy link
Contributor

craig bot commented Jul 14, 2023

Build succeeded:

@craig craig bot merged commit 302310d into cockroachdb:master Jul 14, 2023
@pjtatlow pjtatlow deleted the support-custom-crash-report-tags branch July 17, 2023 15:16
@pjtatlow pjtatlow added the backport-23.1.x Flags PRs that need to be backported to 23.1 label Jul 17, 2023
@jaylim-crl
Copy link
Collaborator

blathers backport 23.1

craig bot pushed a commit that referenced this pull request Aug 17, 2023
In #106786 we added the ability to provide an environment
variable that was meant to add custom tags to sentry crash
reports. That change added the function that would create
the map of crash report tags / values, but it was never actually
used. This change ensures that tags from that environment
variable will actually show up in the sentry reports.

Release note: None

Epic: None
craig bot pushed a commit that referenced this pull request Aug 17, 2023
107394: cmd/roachtest: add disagg-rebalance roachtest r=renatolabs a=itsbilal

This test adds a roachtest that spins up a cluster with 3 nodes using S3 as the --experimental-shared-storage, and then adds a fourth node after loading a tpcc fixture and with a foreground workload running on it. It confirms the fourth node gets hydrated without transferring all live bytes over the wire.

Epic: none
Fixes: #103030

Release note: None

108154: kvcoord: refactor ambiguous commit tests r=AlexTalks a=AlexTalks

In #107323, testing for the ambiguous write case that leads to the "transaction unexpectedly committed" bug were introduced, however to increase test coverage of the fix, multiple schedules of operations need to be tested. This change simply refactors the framework of the existing test in order to enable the addition of muliple subtests. The subtests are included in a separate patch.

Part of: #103817

Release note: None

108819: roachtest: add a c2c cutover `TO LATEST` test r=lidorcarmel a=lidorcarmel

We only have c2c roachtests that cutover to the past, adding one that does a cutover to LATEST. Using the `TO LATEST` sql because we expect that to be used more in production.

Epic: none

Release note: None

108910: streamingccl: minor log updates and code reorg r=lidorcarmel a=stevendanna

See individual commits.

Epic: none

108914: sqlproxyccl: do not report BackendDown metrics on throttle and routing errors r=JeffSwenson,andy-kimball a=jaylim-crl

#### sqlproxyccl: do not report BackendDown metrics on throttle and routing errors

Previously, we were reporting the backend_down metric on the following errors:
- codeProxyRefusedConnection
- codeParamsRoutingFailed
- codeUnavailable

These errors do not imply that the backend is down. We originally introduced
this in #57431, but looking at the PR, it appears unintentional. This commit
fixes that by not reporting the backend_down metric when the proxy returns
such errors.

Release note: None

Epic: none

#### sqlproxyccl: rename codeBackendDown to codeBackendDialFailed

This commit renames codeBackendDown to codeBackendDialFailed to prevent
confusions by developers. Note that we don't rename the metric here to avoid
breaking downstream consumers. At the same time, we will remove the old
codeBackendRefusedTLS code as it does not serve any purpose, and there wasn't
a metric for it as well.

Release note: None

Epic: none



Release justification: This fixes accuracy issues with SQL Proxy metrics.

108920: util/log: add custom crash tags to sentry r=dhartunian a=pjtatlow

In #106786 we added the ability to provide an environment variable that was meant to add custom tags to sentry crash reports. That change added the function that would create the map of crash report tags / values, but it was never actually used. This change ensures that tags from that environment variable will actually show up in the sentry reports.

Release note: None

Epic: None

Co-authored-by: Bilal Akhtar <[email protected]>
Co-authored-by: Alex Sarkesian <[email protected]>
Co-authored-by: Lidor Carmel <[email protected]>
Co-authored-by: Steven Danna <[email protected]>
Co-authored-by: Jay <[email protected]>
Co-authored-by: PJ Tatlow <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-23.1.x Flags PRs that need to be backported to 23.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants