refactor: logging standardisation by james-milligan · Pull Request #207 · open-feature/flagd

james-milligan · 2022-11-07T15:34:51Z

This PR

introduce logging service to wrap zap logs
standardise logging pattern across services

Related Issues

#184

Notes

Follow-up Tasks

How to test

Signed-off-by: James Milligan <james@omnant.co.uk>

james-milligan · 2022-11-07T16:42:20Z

This fails on benchmarking however I believe this is to be expected with the addition of the new logs

toddbaert · 2022-11-09T16:55:16Z

This fails on benchmarking however I believe this is to be expected with the addition of the new logs

@james-milligan @skyerus @AlexsJones @beeme1mr this comment made me wonder:

Could we implement async logging? We want flagd to be as low-latency as possible - I think it would be nice to make all the logging async. It looks like logrus doesn't support this out of the box, but there's a more-or-less standard solution:

sirupsen/logrus#725

toddbaert · 2022-11-09T16:55:26Z

@james-milligan this is a good start!

AlexsJones · 2022-11-09T17:07:41Z

This fails on benchmarking however I believe this is to be expected with the addition of the new logs

@james-milligan @skyerus @AlexsJones @beeme1mr this comment made me wonder:

Could we implement async logging? We want flagd to be as low-latency as possible - I think it would be nice to make all the logging async. It looks like logrus doesn't support this out of the box, but there's a more-or-less standard solution:

sirupsen/logrus#725

I am not really sure if it's going to be worth it as long as we can disable logging if required. I don't mind checking it out though.

Signed-off-by: James Milligan <james@omnant.co.uk>

AlexsJones · 2022-11-17T11:28:22Z

Is this ready to review @james-milligan , should we just ignore the benchmark?

james-milligan · 2022-11-17T11:31:08Z

Is this ready to review @james-milligan , should we just ignore the benchmark?

yes its ready for revie, I think we should ignore the benchmark, it may even be worthwhile updating the benchmark to be more inline with the e2e tests so we can view performance impacts across the full flag evaluation process, not just across the span of the connect service.

If we do find that performance becomes an issue we can introduce an option to noop the logging service for specific loggers, e.g. silence request logs and only show configuration changes, which would also bypass the setting of values in the logger

Signed-off-by: James Milligan <james@omnant.co.uk>

toddbaert · 2022-11-17T17:39:40Z

This fails on benchmarking however I believe this is to be expected with the addition of the new logs

@james-milligan @skyerus @AlexsJones @beeme1mr this comment made me wonder:
Could we implement async logging? We want flagd to be as low-latency as possible - I think it would be nice to make all the logging async. It looks like logrus doesn't support this out of the box, but there's a more-or-less standard solution:
sirupsen/logrus#725

I am not really sure if it's going to be worth it as long as we can disable logging if required. I don't mind checking it out though.

Ya I think I agree. If we can disabling the logging in hot paths (evaluation) but keep it in less hot but important path (flag updates) that's fine with me. I think that will happen on a subsequent PR, cc @james-milligan

Signed-off-by: James Milligan <james@omnant.co.uk>

toddbaert · 2022-11-21T20:23:58Z

Is this ready to review @james-milligan , should we just ignore the benchmark?

yes its ready for revie, I think we should ignore the benchmark, it may even be worthwhile updating the benchmark to be more inline with the e2e tests so we can view performance impacts across the full flag evaluation process, not just across the span of the connect service.

If we do find that performance becomes an issue we can introduce an option to noop the logging service for specific loggers, e.g. silence request logs and only show configuration changes, which would also bypass the setting of values in the logger

I'm in favor of merging this and creating a new issue for a noop logger for evaluations that can be configured to still log update events.

Signed-off-by: Adam Gardner <26523841+agardnerIT@users.noreply.github.com>

…edback - github.com/sigstore/sigstore-go v1.1.4 -> v1.2.2 in test/integration (medium, alert #260) - js-yaml ^4.1.1 -> ^4.2.0 and vite ^6.4.2 -> ^6.4.3 in playground-app (medium/high, alerts #207, #199, #200); lockfile already resolved these transitively, this pins the direct specifiers to match - Root-cause fix for a regression introduced by the prior testcontainers-go v0.41.0 -> v0.43.0 bump: v0.43.0's modules/compose changed DockerContainer.MappedPort's signature (nat.Port -> string), which is incompatible with go-sdk-contrib/tests/flagd/v2@v2.0.2's testframework (pinned, no newer version exists) and broke `go vet`/`go test` compilation in test/integration (go build alone didn't catch it since _test.go files aren't compiled by `go build`). Reverted testcontainers-go and testcontainers-go/modules/compose to v0.41.0 (API-compatible) while keeping the already-patched transitive containerd/containerd/v2 v2.2.5 and other bumps pinned via explicit go.mod requires, so alerts #189, #192-#196 remain resolved without reintroducing the vulnerable containerd version. - Add ReadTimeout to the flagd-proxy metrics server's http.Server per CodeRabbit review feedback on this PR: ReadHeaderTimeout alone only bounds header parsing, leaving the plain HTTP/h2c port open to slow-client (Slowloris-style) resource exhaustion. Still unresolvable: github.com/docker/docker (alerts #151, #152, #184, #185, #186) remains at v28.5.2+incompatible in test/integration. No version >= 28.5.2 (including the advisory's referenced 29.3.1 for alert #152) is published under the legacy github.com/docker/docker import path in the Go module proxy; moby now ships new majors under github.com/moby/moby/v2, which this module graph cannot switch to without deeper dependency surgery. Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>

## Summary Resolves all cleanly patchable open Dependabot security alerts (28 of 33). The remaining 5 have no available patched version for their legacy import path — see below. ## Dependabot Alerts Resolved | Alert | Package | Severity | Fix | |-------|---------|----------|-----| | #260 | `github.com/sigstore/sigstore-go` | **medium** | Bumped to v1.2.2 in `test/integration` | | #247-#259 (13 alerts) | `golang.org/x/crypto` | **critical/high/medium** | Already resolved on `main` (v0.52.0+); carried through the rebase | | #207 | `js-yaml` | **medium** | Bumped to `^4.2.0` in `playground-app` | | #206, #202, #201 | `golang.org/x/net` | **medium** | Bumped to v0.55.0/v0.56.0 across `flagd`, `core`, `flagd-proxy`, `test/integration`, and related test modules | | #200, #199 | `vite` | **medium/high** | Bumped to `^6.4.3` in `playground-app` | | #197 | `@babel/core` | **low** | Forced `>=7.29.6` via npm override in `playground-app` | | #189, #192-#196 | `github.com/containerd/containerd/v2` | **high/medium** | Bumped to v2.2.5 in `test/integration` (kept on `testcontainers-go` v0.41.0 — see note below) | | #190 | `js-cookie` | **high** | Forced `>=3.0.7` via npm override in `playground-app` | **Note on containerd fix:** an earlier revision of this branch bumped `testcontainers-go` to v0.43.0 to reach patched `containerd/v2`, but that also changed `DockerContainer.MappedPort`'s signature in a way that's incompatible with `go-sdk-contrib/tests/flagd/v2@v2.0.2` (pinned; no newer version exists) — breaking `go vet`/`go test` compilation in `test/integration` (not caught by `go build` alone, since it doesn't compile `_test.go` files). Reverted `testcontainers-go` to v0.41.0 (API-compatible) while keeping `containerd/containerd/v2` pinned at v2.2.5 via an explicit `go.mod` require — standard Go MVS, no `replace` directive needed. ## Unresolvable Alerts **`github.com/docker/docker`** (#151, #152, #184, #185, #186) in `test/integration`: pinned at `v28.5.2+incompatible`. No version ≥ that (including the advisory's referenced `v29.3.1` for #152) exists under the legacy `github.com/docker/docker` import path in the Go module proxy — moby now ships new majors under `github.com/moby/moby/v2`. Transitive dependency of testcontainers/buildx/compose; unresolvable until those upstreams migrate. | Alert | Package | Severity | Reason | |-------|---------|----------|--------| | #186 | `github.com/docker/docker` | **high** | No patched version in Go module proxy | | #185 | `github.com/docker/docker` | **medium** | No patched version in Go module proxy | | #184 | `github.com/docker/docker` | **high** | No patched version in Go module proxy | | #152 | `github.com/docker/docker` | **high** | Advisory's patched version (29.3.1) not published for this import path | | #151 | `github.com/docker/docker` | **medium** | No patched version in Go module proxy | ## Other changes - Added `ReadTimeout` to the flagd-proxy metrics server's `http.Server` (`flagd-proxy/pkg/service/server.go`) per CodeRabbit review feedback, hardening the plain HTTP/h2c port against slow-client resource exhaustion. ## Verification - `make test`, `make build`, and `go vet` pass across all Go modules (`core`, `flagd`, `flagd-proxy`, `flagd-proxy/tests/loadtest`, `test/zero-downtime-flagd-proxy`). - `test/integration`: `go build ./...`, `go vet ./...`, and `go test` (compile-only; actual container run needs a Docker daemon) all pass. - `playground-app`: `npm run build` succeeds; `npm install --package-lock-only --dry-run` reports up to date. --------- Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com> Signed-off-by: Todd Baert <todd.baert@dynatrace.com> Co-authored-by: Todd Baert <todd.baert@dynatrace.com>

james-milligan added 8 commits November 7, 2022 15:16

logging refactor

8637d87

Signed-off-by: James Milligan <james@omnant.co.uk>

updated service logging

4a2a4ea

Signed-off-by: James Milligan <james@omnant.co.uk>

added request id to logging output

47620d9

Signed-off-by: James Milligan <james@omnant.co.uk>

fixed additional log inconsistency

002a9d4

Signed-off-by: James Milligan <james@omnant.co.uk>

refactor log

11465c2

Signed-off-by: James Milligan <james@omnant.co.uk>

test fixes

21b8c92

Signed-off-by: James Milligan <james@omnant.co.uk>

gofumpt

6344f31

Signed-off-by: James Milligan <james@omnant.co.uk>

removed dupl linting form services

c9e7d1f

Signed-off-by: James Milligan <james@omnant.co.uk>

toddbaert requested review from beeme1mr, skyerus and toddbaert November 9, 2022 16:50

james-milligan added 13 commits November 14, 2022 10:28

removed remaining fmt.Print calls

73aa9d3

Signed-off-by: James Milligan <james@omnant.co.uk>

replaced os.Exit following error calls with log.fatal

3e2f446

Signed-off-by: James Milligan <james@omnant.co.uk>

zap refactor

34ed0b5

Signed-off-by: James Milligan <james@omnant.co.uk>

zap logger sync fix

34677a2

Signed-off-by: James Milligan <james@omnant.co.uk>

removed dev comments

3e2c2dd

Signed-off-by: James Milligan <james@omnant.co.uk>

error catch and refactor

bf530fe

Signed-off-by: James Milligan <james@omnant.co.uk>

logging refactor

215b014

Signed-off-by: James Milligan <james@omnant.co.uk>

removed filepath sync

3557077

Signed-off-by: James Milligan <james@omnant.co.uk>

logger format and linting fix

1170163

Signed-off-by: James Milligan <james@omnant.co.uk>

missed log line

ac435c4

Signed-off-by: James Milligan <james@omnant.co.uk>

removed output.txt

8e56cbf

Signed-off-by: James Milligan <james@omnant.co.uk>

logging refactor + abstraction

f5fd116

Signed-off-by: James Milligan <james@omnant.co.uk>

linting

3950bb9

Signed-off-by: James Milligan <james@omnant.co.uk>

james-milligan marked this pull request as ready for review November 16, 2022 16:03

james-milligan requested a review from AlexsJones as a code owner November 16, 2022 16:03

james-milligan added 2 commits November 17, 2022 10:37

added comment describing functionality of package

4825634

Signed-off-by: James Milligan <james@omnant.co.uk>

unit test

b0817b1

Signed-off-by: James Milligan <james@omnant.co.uk>

skyerus reviewed Nov 17, 2022

View reviewed changes

Comment thread pkg/eval/json_evaluator.go

skyerus reviewed Nov 17, 2022

View reviewed changes

Comment thread pkg/logger/logger.go Outdated

skyerus reviewed Nov 17, 2022

View reviewed changes

Comment thread pkg/logger/logger.go Outdated

skyerus reviewed Nov 17, 2022

View reviewed changes

Comment thread pkg/logger/logger.go Outdated

suggested changes

a6c6282

Signed-off-by: James Milligan <james@omnant.co.uk>

toddbaert reviewed Nov 17, 2022

View reviewed changes

Comment thread pkg/sync/kubernetes/kubernetes_sync.go

toddbaert reviewed Nov 17, 2022

View reviewed changes

Comment thread pkg/logger/logger.go

toddbaert self-requested a review November 17, 2022 17:39

toddbaert approved these changes Nov 17, 2022

View reviewed changes

Kavindu-Dodan reviewed Nov 18, 2022

View reviewed changes

Comment thread cmd/start.go Outdated

kube notifier event log

8995ec4

Signed-off-by: James Milligan <james@omnant.co.uk>

skyerus reviewed Nov 18, 2022

View reviewed changes

Comment thread pkg/logger/logger.go

james-milligan added 2 commits November 18, 2022 16:07

moved zap log generation to function

28cdbf9

Signed-off-by: James Milligan <james@omnant.co.uk>

introduce log check level

1f8b834

Signed-off-by: James Milligan <james@omnant.co.uk>

toddbaert approved these changes Nov 21, 2022

View reviewed changes

beeme1mr approved these changes Nov 22, 2022

View reviewed changes

beeme1mr merged commit 418b9ef into open-feature:main Nov 22, 2022

james-milligan deleted the logging branch March 23, 2023 10:58

raphael-wigoutschnigg-dt pushed a commit to open-feature-forking/flagd that referenced this pull request Mar 11, 2025

docs: update flagd port in readme (open-feature#207)

919d808

Signed-off-by: Adam Gardner <26523841+agardnerIT@users.noreply.github.com>

jonathannorris mentioned this pull request Jul 20, 2026

fix: add server ReadTimeouts, update security deps #1980

Merged

Navigation Menu

refactor: logging standardisation - #207

refactor: logging standardisation#207
beeme1mr merged 30 commits into
open-feature:mainfrom
james-milligan:logging

james-milligan commented Nov 7, 2022 •

edited

Loading

Uh oh!

james-milligan commented Nov 7, 2022

Uh oh!

toddbaert commented Nov 9, 2022 •

edited

Loading

Uh oh!

toddbaert commented Nov 9, 2022

Uh oh!

AlexsJones commented Nov 9, 2022

Uh oh!

AlexsJones commented Nov 17, 2022

Uh oh!

james-milligan commented Nov 17, 2022 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

toddbaert commented Nov 17, 2022

Uh oh!

Uh oh!

Uh oh!

toddbaert commented Nov 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

james-milligan commented Nov 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

This PR

Related Issues

Notes

Follow-up Tasks

How to test

Uh oh!

james-milligan commented Nov 7, 2022

Uh oh!

toddbaert commented Nov 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

toddbaert commented Nov 9, 2022

Uh oh!

AlexsJones commented Nov 9, 2022

Uh oh!

AlexsJones commented Nov 17, 2022

Uh oh!

james-milligan commented Nov 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

toddbaert commented Nov 17, 2022

Uh oh!

Uh oh!

Uh oh!

toddbaert commented Nov 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

james-milligan commented Nov 7, 2022 •

edited

Loading

toddbaert commented Nov 9, 2022 •

edited

Loading

james-milligan commented Nov 17, 2022 •

edited

Loading