Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Log pkg/errors stack traces #5076

Merged
merged 1 commit into from
Nov 6, 2023
Merged

Log pkg/errors stack traces #5076

merged 1 commit into from
Nov 6, 2023

Conversation

MichaelSnowden
Copy link
Contributor

What changed?
I upgraded our zap version from v1.24.0 to v1.26.0, which contains support for pkg/errors. See this issue and this commit.
image

Why?
Before this change, our logs would only contain the stack trace from where the logger itself was invoked, not from the source of where the error was generated or wrapped. This provided very little useful information.

How did you test it?
I ran a custom build of server which overwrote some deep code in history to return an error. I then set up docker-compose to output logs -> promtail -> loki -> grafana. Then, I queried Grafana to verify that the error log contained an "errorVerbose" field with the stack trace from where my error was generated. As you can see from the below image, the stack trace does appear under this field, and if you turn on JSON parsing and newline escaping, you can both see it rendered correctly, and you can copy-paste the stack trace.

image image

The stack trace from Grafana:

oopsie woopsie
main.(*faultyShardEngine).StartWorkflowExecution
	/Users/mikey/src/temporalio/temporal/.scratches/main.go:39
go.temporal.io/server/service/history.(*Handler).StartWorkflowExecution
	/Users/mikey/src/temporalio/temporal/service/history/handler.go:595
go.temporal.io/server/api/historyservice/v1._HistoryService_StartWorkflowExecution_Handler.func1
	/Users/mikey/src/temporalio/temporal/api/historyservice/v1/service.pb.go:1300
...

Potential risks
The stack traces are pretty deep because of all our gRPC interceptors. However, we can definitely fix that later if we want by filtering the pkg/errors.StackTrace. I'd rather do that in a follow-up after getting support for this initial change first, though.

Is hotfix candidate?
No.

@MichaelSnowden MichaelSnowden requested a review from a team as a code owner November 4, 2023 21:46
Copy link
Contributor

@stephanos stephanos left a comment

Choose a reason for hiding this comment

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

❤️

@MichaelSnowden MichaelSnowden merged commit 37ffdd2 into main Nov 6, 2023
@MichaelSnowden MichaelSnowden deleted the snowden/pkg-error-logs branch November 6, 2023 17:28
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants