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

Logging runtime ID and shoot name #521

Merged
merged 11 commits into from
Nov 26, 2024
Merged

Conversation

VOID404
Copy link
Contributor

@VOID404 VOID404 commented Nov 22, 2024

Description

Changes proposed in this pull request:

  • add fields to FSM logs:
    • shoot
    • runtime id
    • request id
  • add logging of patch error
  • change logger from dev to prod
  • log patch error

Related issue(s)

@VOID404 VOID404 requested a review from a team as a code owner November 22, 2024 07:48
@kyma-bot kyma-bot added cla: yes Indicates the PR's author has signed the CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 22, 2024
@VOID404 VOID404 changed the title Logging runtime ID and shoot name [WIP] Logging runtime ID and shoot name Nov 22, 2024
@kyma-bot kyma-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 22, 2024
@VOID404 VOID404 changed the title [WIP] Logging runtime ID and shoot name Logging runtime ID and shoot name Nov 22, 2024
@kyma-bot kyma-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 22, 2024
@VOID404 VOID404 force-pushed the logger/id-field branch 2 times, most recently from a7e62af to 86293a9 Compare November 22, 2024 08:01
Comment on lines 60 to 75
runtimeID, ok := runtime.Labels["kyma-project.io/runtime-id"]
if !ok {
runtimeID = runtime.Name
}

shootName, ok := runtime.Labels["kyma-project.io/shoot-name"]
if !ok {
shootName = "N/D"
}

log := r.Log.WithValues("runtime", runtimeID, "shoot", shootName)
log.Info("Reconciling Runtime", "Name", runtime.Name, "Namespace", runtime.Namespace)
Copy link
Contributor

Choose a reason for hiding this comment

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

The overall idea is nice, and this is the correct place to implement that change. I tested it on local environment at it looks good, but I have one minor remark:

2024-11-22T12:03:04+01:00       INFO    Reconciling Runtime     {"runtime": "aaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", "shoot": "test-load", "Name": "test-ci-m5t28", "Namespace": "kcp-system"}
2024-11-22T12:03:04+01:00       INFO    reqID 0 reconciliation done     {"runtime": "aaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", "shoot": "test-load", "error": null, "result": null}

Could you change in line 70 runtime to runtimeID to indicate that log is printing runtimeID?

@kyma-bot kyma-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 25, 2024
mvshao
mvshao previously approved these changes Nov 26, 2024
@kyma-bot kyma-bot added the lgtm Looks good to me! label Nov 26, 2024
@kyma-bot kyma-bot removed the lgtm Looks good to me! label Nov 26, 2024
@kyma-bot kyma-bot added the lgtm Looks good to me! label Nov 26, 2024
@kyma-bot kyma-bot merged commit e8fe97e into kyma-project:main Nov 26, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indicates the PR's author has signed the CLA. lgtm Looks good to me! size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants