improvement CLDSRV-466 add timestamp for exceptions #5406

tmacro · 2023-11-02T16:35:27Z

Adds a timestamp to uncaught exceptions to aid in debugging.

bert-e · 2023-11-02T16:35:30Z

Hello tmacro,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Status report is not available.

bert-e · 2023-11-02T16:37:12Z

Request integration branches

Waiting for integration branch creation to be requested by the user.

To request integration branches, please comment on this pull request with the following command:

/create_integration_branches

Alternatively, the /approve and /create_pull_requests commands will automatically
create the integration branches.

tmacro · 2023-11-02T16:38:07Z

/create_integration_branches

bert-e · 2023-11-02T16:38:23Z

Integration data created

I have created the integration data for the additional destination branches.

this pull request will merge improvement/CLDSRV-466/timestamps_in_stderr into
development/7.10
w/7.70/improvement/CLDSRV-466/timestamps_in_stderr will be merged into development/7.70
w/8.6/improvement/CLDSRV-466/timestamps_in_stderr will be merged into development/8.6
w/8.7/improvement/CLDSRV-466/timestamps_in_stderr will be merged into development/8.7
w/8.8/improvement/CLDSRV-466/timestamps_in_stderr will be merged into development/8.8

The following branches will NOT be impacted:

development/7.4

You can set option create_pull_requests if you need me to create
integration pull requests in addition to integration branches, with:

@bert-e create_pull_requests

The following options are set: create_integration_branches

bert-e · 2023-11-02T16:38:26Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
2 peers

The following options are set: create_integration_branches

jonathan-gramain · 2023-11-02T17:13:50Z

index.js

+ * Catch uncaught exceptions and add timestamp to aid debugging
+ */
+process.on('uncaughtException', err => {
+    process.stderr.write(`${new Date().toISOString()} ${err.stack}`);


Since the exception backtrace is multi-line, I'd recommend for readability to add short context info around the timestamp, followed by a newline, then the exception backtrace. This way the first backtrace line is aligned with the other lines.

Here's what I'm thinking about:

process.stderr.write(`${new Date().toISOString()}: Uncaught exception: \n${err.stack}`);

williamlardier · 2023-11-03T16:20:47Z

index.js

+/**
+ * Catch uncaught exceptions and add timestamp to aid debugging
+ */
+process.on('uncaughtException', err => {


Do we want to also catch the rejections?

It's been a long time but this does catch rejections as well

williamlardier · 2023-11-03T16:44:00Z

index.js

+ * Catch uncaught exceptions and add timestamp to aid debugging
+ */
+process.on('uncaughtException', err => {
+    process.stderr.write(`${new Date().toISOString()}: Uncaught exception: \n${err.stack}`);
+});


This will change the service behavior in case of error right? Like, we won't crash, and we won't end the current request either, is that what we want? Maybe I'm missing something here?

That's what I thought too, but from observation he worker still exits after the message is printed.

old PR, but the worker exits because he adds its own listener (

cloudserver/lib/server.js

Lines 82 to 91 in 4f7aa54

process.on('uncaughtException', err => {

// If just send the error object results in empty

// object on server log.

logger.fatal('caught error', {

error: err.message,

stack: err.stack,

workerId: this.worker ? this.worker.id : undefined,

workerPid: this.worker ? this.worker.process.pid : undefined,

});

this.caughtExceptionShutdown();

) to shutdown after this one.

However the primary (that uses fork and has the monitoring server for prom metrics) don't have another listener, so this is keep the primary up in undefined state if an exception is caught

tmacro · 2023-11-10T16:17:55Z

/reset

bert-e · 2023-11-10T16:18:34Z

Reset complete

I have successfully deleted this pull request's integration branches.

The following options are set: create_integration_branches

bert-e · 2023-11-10T16:18:47Z

Integration data created

I have created the integration data for the additional destination branches.

this pull request will merge improvement/CLDSRV-466/timestamps_in_stderr into
development/7.10
w/7.70/improvement/CLDSRV-466/timestamps_in_stderr will be merged into development/7.70
w/8.6/improvement/CLDSRV-466/timestamps_in_stderr will be merged into development/8.6
w/8.7/improvement/CLDSRV-466/timestamps_in_stderr will be merged into development/8.7
w/8.8/improvement/CLDSRV-466/timestamps_in_stderr will be merged into development/8.8

The following branches will NOT be impacted:

development/7.4

You can set option create_pull_requests if you need me to create
integration pull requests in addition to integration branches, with:

@bert-e create_pull_requests

The following options are set: create_integration_branches

bert-e · 2023-11-10T16:18:48Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
2 peers

The following options are set: create_integration_branches

tmacro · 2023-11-13T17:19:57Z

/approve

bert-e · 2023-11-13T17:20:20Z

In the queue

The changeset has received all authorizations and has been added to the
relevant queue(s). The queue(s) will be merged in the target development
branch(es) as soon as builds have passed.

The changeset will be merged in:

✔️ development/7.10
✔️ development/7.70
✔️ development/8.6
✔️ development/8.7
✔️ development/8.8

The following branches will NOT be impacted:

development/7.4

There is no action required on your side. You will be notified here once
the changeset has been merged. In the unlikely event that the changeset
fails permanently on the queue, a member of the admin team will
contact you to help resolve the matter.

IMPORTANT

Please do not attempt to modify this pull request.

Any commit you add on the source branch will trigger a new cycle after the
current queue is merged.
Any commit you add on one of the integration branches will be lost.

If you need this pull request to be removed from the queue, please contact a
member of the admin team now.

The following options are set: approve, create_integration_branches

bert-e · 2023-11-13T17:38:16Z

I have successfully merged the changeset of this pull request
into targetted development branches:

✔️ development/7.10
✔️ development/7.70
✔️ development/8.6
✔️ development/8.7
✔️ development/8.8

The following branches have NOT changed:

development/7.4

Please check the status of the associated issue CLDSRV-466.

Goodbye tmacro.

scality deleted a comment from bert-e Nov 2, 2023

jonathan-gramain reviewed Nov 2, 2023

View reviewed changes

jonathan-gramain approved these changes Nov 3, 2023

View reviewed changes

williamlardier reviewed Nov 3, 2023

View reviewed changes

williamlardier approved these changes Nov 7, 2023

View reviewed changes

improvement CLDSRV-466 add timestamp for exceptions

b1b2d2a

tmacro force-pushed the improvement/CLDSRV-466/timestamps_in_stderr branch from 1f73366 to b1b2d2a Compare November 10, 2023 16:17

scality deleted a comment from bert-e Nov 10, 2023

bert-e merged commit 0882bff into development/7.10 Nov 13, 2023
10 checks passed

bert-e deleted the improvement/CLDSRV-466/timestamps_in_stderr branch November 13, 2023 17:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improvement CLDSRV-466 add timestamp for exceptions #5406

improvement CLDSRV-466 add timestamp for exceptions #5406

tmacro commented Nov 2, 2023

bert-e commented Nov 2, 2023

bert-e commented Nov 2, 2023

tmacro commented Nov 2, 2023

bert-e commented Nov 2, 2023

bert-e commented Nov 2, 2023

jonathan-gramain Nov 2, 2023

williamlardier Nov 3, 2023

BourgoisMickael Jun 21, 2024

williamlardier Nov 3, 2023

tmacro Nov 6, 2023

BourgoisMickael Jun 24, 2024

tmacro commented Nov 10, 2023

bert-e commented Nov 10, 2023

bert-e commented Nov 10, 2023

bert-e commented Nov 10, 2023

tmacro commented Nov 13, 2023

bert-e commented Nov 13, 2023

bert-e commented Nov 13, 2023

	process.on('uncaughtException', err => {
	// If just send the error object results in empty
	// object on server log.
	logger.fatal('caught error', {
	error: err.message,
	stack: err.stack,
	workerId: this.worker ? this.worker.id : undefined,
	workerPid: this.worker ? this.worker.process.pid : undefined,
	});
	this.caughtExceptionShutdown();

improvement CLDSRV-466 add timestamp for exceptions #5406

improvement CLDSRV-466 add timestamp for exceptions #5406

Conversation

tmacro commented Nov 2, 2023

bert-e commented Nov 2, 2023

Hello tmacro,

bert-e commented Nov 2, 2023

Request integration branches

tmacro commented Nov 2, 2023

bert-e commented Nov 2, 2023

Integration data created

bert-e commented Nov 2, 2023

Waiting for approval

jonathan-gramain Nov 2, 2023

Choose a reason for hiding this comment

williamlardier Nov 3, 2023

Choose a reason for hiding this comment

BourgoisMickael Jun 21, 2024

Choose a reason for hiding this comment

williamlardier Nov 3, 2023

Choose a reason for hiding this comment

tmacro Nov 6, 2023

Choose a reason for hiding this comment

BourgoisMickael Jun 24, 2024

Choose a reason for hiding this comment

tmacro commented Nov 10, 2023

bert-e commented Nov 10, 2023

Reset complete

bert-e commented Nov 10, 2023

Integration data created

bert-e commented Nov 10, 2023

Waiting for approval

tmacro commented Nov 13, 2023

bert-e commented Nov 13, 2023

In the queue

bert-e commented Nov 13, 2023