Skip to content

Comments

K8SPG-903: replace "errors" with "github.com/pkg/errors"#1436

Open
pooknull wants to merge 9 commits intomainfrom
K8SPG-903
Open

K8SPG-903: replace "errors" with "github.com/pkg/errors"#1436
pooknull wants to merge 9 commits intomainfrom
K8SPG-903

Conversation

@pooknull
Copy link
Contributor

@pooknull pooknull commented Feb 11, 2026

https://perconadev.atlassian.net/browse/K8SPG-903

DESCRIPTION

This PR improves operator logs by adding stack traces to all errors.

There are multiple ways to achieve this. For example, the Crunchy codebase already uses standard errors.WithStack(). However, in some parts of the code it is missing, which results in the operator logging errors without stack traces. To ensure stack traces using this approach, we would need to wrap every returned error with errors.WithStack(), which is easy to forget and may be missed in the future.

In our other operators, the common practice is to use the github.com/pkg/errors package, which automatically includes a stack trace when errors are created or wrapped. By replacing "errors" with "github.com/pkg/errors", we can ensure that every error contains a stack trace.

CHECKLIST

Jira

  • Is the Jira ticket created and referenced properly?
  • Does the Jira ticket have the proper statuses for documentation (Needs Doc) and QA (Needs QA)?
  • Does the Jira ticket link to the proper milestone (Fix Version field)?

Tests

  • Is an E2E test/test case added for the new feature/change?
  • Are unit tests added where appropriate?

Config/Logging/Testability

  • Are all needed new/changed options added to default YAML files?
  • Are all needed new/changed options added to the Helm Chart?
  • Did we add proper logging messages for operator actions?
  • Did we ensure compatibility with the previous version or cluster upgrade process?
  • Does the change support oldest and newest supported PG version?
  • Does the change support oldest and newest supported Kubernetes version?

Copilot AI review requested due to automatic review settings February 11, 2026 13:55
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot AI review requested due to automatic review settings February 12, 2026 09:27
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 63 out of 63 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot AI review requested due to automatic review settings February 12, 2026 13:03
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 63 out of 63 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@pooknull pooknull marked this pull request as ready for review February 13, 2026 10:51
Copilot AI review requested due to automatic review settings February 13, 2026 10:51
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 63 out of 63 changed files in this pull request and generated 2 comments.

import (
"context"
"errors"
"fmt"
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

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

The fmt package is imported but no longer used in this file after replacing fmt.Errorf with errors.Errorf. Remove the unused import.

Suggested change
"fmt"

Copilot uses AI. Check for mistakes.
@@ -6,7 +6,6 @@ package postgrescluster

import (
"context"
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

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

The fmt package is imported but no longer used in this file after replacing fmt.Errorf with errors.Errorf. Remove the unused import.

Copilot uses AI. Check for mistakes.
@gkech
Copy link
Contributor

gkech commented Feb 16, 2026

we still have some usages of fmt.Errorf through a recently merged PR:

Screenshot 2026-02-16 at 2 09 49 PM

@egegunes
Copy link
Contributor

egegunes commented Feb 17, 2026

I am not sure if these changes fix the actual problem. Because we have stacktraces in all errors, the problem is that stacktrace only contains traces from controller-runtime.

@gkech
Copy link
Contributor

gkech commented Feb 18, 2026

I am not sure if these changes fix the actual problem. Because we have stacktraces in all errors, the problem is that stacktrace only contains traces from controller-runtime.

we can trigger some errors from the upstream implementation and ensure that they have the full stacktraces, right?

@pooknull
Copy link
Contributor Author

@egegunes @gkech

The primary ways to capture a stack trace in Go are by calling:

These functions capture the stack trace at the point where they are called, not at the original source of the error.

The reason we see a short stack trace for each error in controller-runtime is that it calls log.Error here:
https://github.com/kubernetes-sigs/controller-runtime/blob/e4c1c38bcbdb6e37d85481909787891a6ffc0a67/pkg/internal/controller/controller.go#L495
The zap logger internally calls runtime.Callers when emitting a log entry. The stack trace we see corresponds to the location where log.Error was invoked and not where the error originally occurred.

If we want stack traces that point to where the error actually originated, we must capture the stack at the moment the error is created or wrapped. The github.com/pkg/errors package does exactly this. It calls the stack capture functions inside:

  • errors.New
  • errors.Wrap
  • errors.WithStack
  • ...

Each of these attaches a stack trace to the error at creation time. When such an error is logged with zap, the attached stack trace is visible in the errorVerbose field (this is equivalent to calling fmt.Printf("%+v", err)).

Copilot AI review requested due to automatic review settings February 18, 2026 11:23
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 64 out of 64 changed files in this pull request and generated 11 comments.

Copy link
Contributor

@egegunes egegunes left a comment

Choose a reason for hiding this comment

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

LGTM, @pooknull thanks for the explanation.

@egegunes egegunes added this to the v2.9.0 milestone Feb 19, 2026
@JNKPercona
Copy link
Collaborator

Test Name Result Time
backup-enable-disable passed 00:00:00
builtin-extensions passed 00:00:00
custom-envs passed 00:18:43
custom-extensions passed 00:00:00
custom-tls passed 00:00:00
database-init-sql passed 00:00:00
demand-backup passed 00:00:00
demand-backup-offline-snapshot passed 00:00:00
dynamic-configuration passed 00:00:00
finalizers passed 00:00:00
init-deploy passed 00:00:00
huge-pages passed 00:00:00
monitoring passed 00:00:00
monitoring-pmm3 passed 00:00:00
one-pod passed 00:00:00
operator-self-healing passed 00:00:00
pitr passed 00:00:00
scaling passed 00:00:00
scheduled-backup passed 00:00:00
self-healing passed 00:00:00
sidecars passed 00:00:00
standby-pgbackrest passed 00:00:00
standby-streaming passed 00:00:00
start-from-backup passed 00:00:00
tablespaces passed 00:00:00
telemetry-transfer passed 00:00:00
upgrade-consistency passed 00:00:00
upgrade-minor passed 00:00:00
users passed 00:00:00
Summary Value
Tests Run 29/29
Job Duration 00:33:07
Total Test Time 00:18:43

commit: 647b112
image: perconalab/percona-postgresql-operator:PR-1436-647b11269

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants