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

feat(logger): add support for log tags #1027

Merged
merged 10 commits into from
Jan 8, 2025
Merged

feat(logger): add support for log tags #1027

merged 10 commits into from
Jan 8, 2025

Conversation

john-scalingo
Copy link
Contributor

@john-scalingo john-scalingo commented Dec 30, 2024

  • Add a changelog entry in CHANGELOG.md

@john-scalingo john-scalingo self-assigned this Dec 30, 2024
@john-scalingo john-scalingo force-pushed the experiment/loggable branch 2 times, most recently from 5a13515 to fea09fc Compare December 30, 2024 14:57
@EtienneM
Copy link
Member

For the record, I asked for feedbacks here: https://scalingo.slack.com/archives/C67P0UZ1C/p1735571253973889

@john-scalingo john-scalingo changed the title expermient(logs) Add support log tags experiment(logs) Add support log tags Dec 30, 2024
@EtienneM EtienneM changed the title experiment(logs) Add support log tags feat(logger): add support for log tags Dec 31, 2024
@EtienneM EtienneM force-pushed the experiment/loggable branch from fea09fc to d3b0a4d Compare January 7, 2025 09:33
@EtienneM EtienneM self-assigned this Jan 7, 2025
@EtienneM EtienneM marked this pull request as ready for review January 7, 2025 10:13
Copy link

@sc-david-voisin sc-david-voisin left a comment

Choose a reason for hiding this comment

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

LGTM :)

@leo-scalingo
Copy link
Contributor

FYI, LogFields is used in the scalingo-github-hook

└> grep -R LogFields 2>/dev/null
controllers/archives_get.go:	log = log.WithFields(r.LogFields())
controllers/repo_link_loader_middleware.go:		log = logger.Get(ctx).WithFields(r.LogFields())
controllers/review_apps_delete.go:	log := logger.Get(ctx).WithFields(reviewApp.LogFields())
controllers/review_apps_index.go:		log := logger.Get(ctx).WithFields(child.LogFields())
controllers/review_apps_index.go:	log := logger.Get(ctx).WithFields(reviewApp.LogFields())
controllers/repo_links_create.go:	log := logger.Get(ctx).WithFields(r.LogFields())
cron/review_apps_deletion.go:		ctx, log := logger.WithFieldsToCtx(ctx, r.LogFields())
models/repo_link.go:func (r RepoLink) LogFields() logrus.Fields {
models/repo_link.go:		ctx, log := logger.WithFieldsToCtx(ctx, child.LogFields())
models/deployment.go:func (d Deployment) LogFields() logrus.Fields {
workers/deployment_status_updated.go:	ctx, _ = logger.WithFieldsToCtx(ctx, deployments[0].LogFields())
workers/deployment_status_updated.go:	log := logger.Get(ctx).WithFields(r.LogFields())
workers/webhooks/commit_status.go:	ctx, _ = logger.WithFieldsToCtx(ctx, currentRepoLink.LogFields())
workers/webhooks/commit_status.go:	ctx, log = logger.WithFieldsToCtx(ctx, deployment.LogFields())
workers/webhooks/pull_request.go:	ctx, log := logger.WithFieldsToCtx(ctx, r.LogFields())
workers/webhooks/push.go:	log := logger.Get(ctx).WithFields(r.LogFields())
workers/webhooks/push.go:		_, log := logger.WithFieldsToCtx(ctx, r.LogFields())
workers/addon_updated.go:	log = logger.Get(ctx).WithFields(r.LogFields())
migrations/20241215_webhook_creation/main.go:			ctx, log := logger.WithFieldsToCtx(ctx, link.LogFields())
reviewapp/pull_request.go:	log = log.WithFields(reviewAppRepoLink.LogFields())
reviewapp/create.go:	log = log.WithFields(r.LogFields())
repo_link/deploy.go:		log.WithFields(deployment.LogFields()).Info("Deployment created waiting for status success")
repo_link/sync.go:			log = log.WithFields(child.LogFields())

Maybe it's more generic than ToLogrusFields

@EtienneM
Copy link
Member

EtienneM commented Jan 7, 2025

FYI, LogFields is used in the scalingo-github-hook

[...]

Maybe it's more generic than ToLogrusFields

And ToLogrusFields is used in LinK (https://github.com/Scalingo/link/blob/master/models/ip.go#L20) :D

Do we want something more generic? I mean, this function returns a logrus.Fields anyway. So it's really specific to this dependency, isn't it?

@john-scalingo
Copy link
Contributor Author

I did not know that LogFields existed. IMHO it's better than ToLogrusFields WDYT ?

@EtienneM
Copy link
Member

EtienneM commented Jan 7, 2025

I did not know that LogFields existed. IMHO it's better than ToLogrusFields WDYT ?

I find both ok. If both of you find LogFields better, then I'll rename :)

edit: done in 2fd757f

Copy link
Contributor

@leo-scalingo leo-scalingo left a comment

Choose a reason for hiding this comment

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

LGTM

@EtienneM EtienneM merged commit 0b37434 into master Jan 8, 2025
3 checks passed
@EtienneM EtienneM deleted the experiment/loggable branch January 8, 2025 09:38
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.

4 participants