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

textlogger: allow caller to override stack unwinding #397

Merged
merged 1 commit into from
Jan 18, 2024

Conversation

pohly
Copy link

@pohly pohly commented Jan 11, 2024

What this PR does / why we need it:

One use case is to use Ginkgo for stack unwinding. Then ginkgo.GingkgoHelper also works for log output, not just for E2E failures.

Special notes for your reviewer:

This is going to be used in extended support for test contexts. A preview of what's to come is currently in https://github.com/kubernetes/kubernetes/pull/122481/commits

Release note:

textlogger: stack unwinding can be implemented outside of klog

One use case is to use Ginkgo for stack unwinding. Then `ginkgo.GingkgoHelper`
also works for log output, not just for E2E failures.
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 11, 2024
@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 11, 2024
@pohly
Copy link
Author

pohly commented Jan 11, 2024

The apidiff errors is something that we can ignore:

 FAIL: k8s.io/klog/v2/textlogger contains incompatible changes:
- Config: old is comparable, new is not
- configOptions: old is comparable, new is not

We never said that the struct with no exported members is comparable and I can't think of a reason why someone would compare them anyway.

@dashpole
Copy link

/assign @mengjiao-liu
/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 11, 2024
@pohly
Copy link
Author

pohly commented Jan 12, 2024

@harshanarayana : can you perhaps take a look?

@harshanarayana
Copy link

@pohly ack

@pohly
Copy link
Author

pohly commented Jan 17, 2024

@harshanarayana : gentle ping 😄

I have some changes pending which depend on this.

_, file, line, ok := runtime.Caller(l.callDepth + 2)
if !ok {
skip := l.callDepth + 2
file, line := l.config.co.unwind(skip)

Choose a reason for hiding this comment

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

Do you think it is a good idea to have an if not nil check for the l.config.co.unwind ? I don't think anyone uses &Config{} to create the config. But just in case if they do and forget to call the Backtrace handler and set the unwind this might run into an issue. or do you think this is an overkill to add this check ?

Choose a reason for hiding this comment

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

@pohly Just a minor comment. I am good otherwise.

Copy link
Author

Choose a reason for hiding this comment

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

My initial reaction was that someone might indeed be doing that because we can't prevent it. But then I remembered that it's explicitly not allowed:

// Must be constructed with NewConfig.
type Config struct {

If we were to add a nil check now for members, then we would enter a slippery slope of supporting something that wasn't meant to be supported and make the code more complex. Besides, Config.output also doesn't have a nil check, so someone who isn't following the documentation is already crashing.

No nil check?

Choose a reason for hiding this comment

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

That is a valid point. I think we can avoid the nil check then

@harshanarayana
Copy link

/lgtm

/cc @dims

@k8s-ci-robot k8s-ci-robot requested a review from dims January 18, 2024 11:38
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 18, 2024
@dims
Copy link
Member

dims commented Jan 18, 2024

/approve
/lgtm
/skip

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, harshanarayana, pohly

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@pohly
Copy link
Author

pohly commented Jan 18, 2024

@dims : you might have to merge manually?

@dims dims merged commit 007e661 into kubernetes:main Jan 18, 2024
16 of 17 checks passed
jooola referenced this pull request in hetznercloud/hcloud-cloud-controller-manager Jan 19, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [k8s.io/klog/v2](https://togithub.com/kubernetes/klog) | `v2.120.0` ->
`v2.120.1` |
[![age](https://developer.mend.io/api/mc/badges/age/go/k8s.io%2fklog%2fv2/v2.120.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/go/k8s.io%2fklog%2fv2/v2.120.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/go/k8s.io%2fklog%2fv2/v2.120.0/v2.120.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/go/k8s.io%2fklog%2fv2/v2.120.0/v2.120.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>kubernetes/klog (k8s.io/klog/v2)</summary>

###
[`v2.120.1`](https://togithub.com/kubernetes/klog/releases/tag/v2.120.1):
Prepare klog release for Kubernetes v1.30 (Take 2)

[Compare
Source](https://togithub.com/kubernetes/klog/compare/v2.120.0...v2.120.1)

##### What's Changed

- textlogger: allow caller to override stack unwinding by
[@&#8203;pohly](https://togithub.com/pohly) in
[https://github.com/kubernetes/klog/pull/397](https://togithub.com/kubernetes/klog/pull/397)

**Full Changelog**:
kubernetes/klog@v2.120.0...v2.120.1

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/hetznercloud/hcloud-cloud-controller-manager).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4xMzUuMCIsInVwZGF0ZWRJblZlciI6IjM3LjEzNS4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
codeboten referenced this pull request in open-telemetry/opentelemetry-collector-contrib Mar 12, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [k8s.io/klog/v2](https://togithub.com/kubernetes/klog) | `v2.110.1` ->
`v2.120.1` |
[![age](https://developer.mend.io/api/mc/badges/age/go/k8s.io%2fklog%2fv2/v2.120.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/go/k8s.io%2fklog%2fv2/v2.120.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/go/k8s.io%2fklog%2fv2/v2.110.1/v2.120.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/go/k8s.io%2fklog%2fv2/v2.110.1/v2.120.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

> [!WARNING]
> Some dependencies could not be looked up. Check the Dependency
Dashboard for more information.

---

### Release Notes

<details>
<summary>kubernetes/klog (k8s.io/klog/v2)</summary>

###
[`v2.120.1`](https://togithub.com/kubernetes/klog/releases/tag/v2.120.1):
Prepare klog release for Kubernetes v1.30 (Take 2)

[Compare
Source](https://togithub.com/kubernetes/klog/compare/v2.120.0...v2.120.1)

#### What's Changed

- textlogger: allow caller to override stack unwinding by
[@&#8203;pohly](https://togithub.com/pohly) in
[https://github.com/kubernetes/klog/pull/397](https://togithub.com/kubernetes/klog/pull/397)

**Full Changelog**:
kubernetes/klog@v2.120.0...v2.120.1

###
[`v2.120.0`](https://togithub.com/kubernetes/klog/releases/tag/v2.120.0):
Prepare klog release for Kubernetes v1.30 (Take 1)

[Compare
Source](https://togithub.com/kubernetes/klog/compare/v2.110.1...v2.120.0)

#### What's Changed

- OWNERS: remove serathius, add mengjiao-liu, promote pohly by
[@&#8203;pohly](https://togithub.com/pohly) in
[https://github.com/kubernetes/klog/pull/394](https://togithub.com/kubernetes/klog/pull/394)
- docs: clarify relationship between different features by
[@&#8203;pohly](https://togithub.com/pohly) in
[https://github.com/kubernetes/klog/pull/395](https://togithub.com/kubernetes/klog/pull/395)
- Add SafePtr wrapper by [@&#8203;kaisoz](https://togithub.com/kaisoz)
in
[https://github.com/kubernetes/klog/pull/393](https://togithub.com/kubernetes/klog/pull/393)
- logr v1.4.1 + SetSlogLogger by
[@&#8203;pohly](https://togithub.com/pohly) in
[https://github.com/kubernetes/klog/pull/396](https://togithub.com/kubernetes/klog/pull/396)

#### New Contributors

- [@&#8203;kaisoz](https://togithub.com/kaisoz) made their first
contribution in
[https://github.com/kubernetes/klog/pull/393](https://togithub.com/kubernetes/klog/pull/393)

**Full Changelog**:
kubernetes/klog@v2.110.1...v2.120.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "on tuesday" (UTC), Automerge - At any
time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/open-telemetry/opentelemetry-collector-contrib).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yMzguMSIsInVwZGF0ZWRJblZlciI6IjM3LjIzOC4xIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: opentelemetrybot <[email protected]>
DougManton referenced this pull request in DougManton/opentelemetry-collector-contrib Mar 13, 2024
…1721)

[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [k8s.io/klog/v2](https://togithub.com/kubernetes/klog) | `v2.110.1` ->
`v2.120.1` |
[![age](https://developer.mend.io/api/mc/badges/age/go/k8s.io%2fklog%2fv2/v2.120.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/go/k8s.io%2fklog%2fv2/v2.120.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/go/k8s.io%2fklog%2fv2/v2.110.1/v2.120.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/go/k8s.io%2fklog%2fv2/v2.110.1/v2.120.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

> [!WARNING]
> Some dependencies could not be looked up. Check the Dependency
Dashboard for more information.

---

### Release Notes

<details>
<summary>kubernetes/klog (k8s.io/klog/v2)</summary>

###
[`v2.120.1`](https://togithub.com/kubernetes/klog/releases/tag/v2.120.1):
Prepare klog release for Kubernetes v1.30 (Take 2)

[Compare
Source](https://togithub.com/kubernetes/klog/compare/v2.120.0...v2.120.1)

#### What's Changed

- textlogger: allow caller to override stack unwinding by
[@&open-telemetry#8203;pohly](https://togithub.com/pohly) in
[https://github.com/kubernetes/klog/pull/397](https://togithub.com/kubernetes/klog/pull/397)

**Full Changelog**:
kubernetes/klog@v2.120.0...v2.120.1

###
[`v2.120.0`](https://togithub.com/kubernetes/klog/releases/tag/v2.120.0):
Prepare klog release for Kubernetes v1.30 (Take 1)

[Compare
Source](https://togithub.com/kubernetes/klog/compare/v2.110.1...v2.120.0)

#### What's Changed

- OWNERS: remove serathius, add mengjiao-liu, promote pohly by
[@&open-telemetry#8203;pohly](https://togithub.com/pohly) in
[https://github.com/kubernetes/klog/pull/394](https://togithub.com/kubernetes/klog/pull/394)
- docs: clarify relationship between different features by
[@&open-telemetry#8203;pohly](https://togithub.com/pohly) in
[https://github.com/kubernetes/klog/pull/395](https://togithub.com/kubernetes/klog/pull/395)
- Add SafePtr wrapper by [@&open-telemetry#8203;kaisoz](https://togithub.com/kaisoz)
in
[https://github.com/kubernetes/klog/pull/393](https://togithub.com/kubernetes/klog/pull/393)
- logr v1.4.1 + SetSlogLogger by
[@&open-telemetry#8203;pohly](https://togithub.com/pohly) in
[https://github.com/kubernetes/klog/pull/396](https://togithub.com/kubernetes/klog/pull/396)

#### New Contributors

- [@&open-telemetry#8203;kaisoz](https://togithub.com/kaisoz) made their first
contribution in
[https://github.com/kubernetes/klog/pull/393](https://togithub.com/kubernetes/klog/pull/393)

**Full Changelog**:
kubernetes/klog@v2.110.1...v2.120.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "on tuesday" (UTC), Automerge - At any
time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/open-telemetry/opentelemetry-collector-contrib).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yMzguMSIsInVwZGF0ZWRJblZlciI6IjM3LjIzOC4xIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: opentelemetrybot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants