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.

Sign up for GitHub

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

Jump to bottom

feat: enable request logging via the --debug flag #226

Merged
merged 14 commits into from
Dec 2, 2022
Merged

feat: enable request logging via the --debug flag #226

merged 14 commits into from
Dec 2, 2022

Conversation

james-milligan
Copy link
Contributor

@james-milligan james-milligan commented Nov 28, 2022 •
edited
Loading

This PR

  • The --debug flag now also controls the logging for requests, when set all logging levels are enabled for the XXXWithID logger methods and allows the setting of request fields, allowing for improved performance when logs are not required.
  • NewLogger now takes an additional boolean argument to set the internal reqIDLogging field, this field is also set to false when the *zap.Logger argument is nil

Related Issues

Notes

Follow-up Tasks

This flag should be set by default in the operator
open-feature/open-feature-operator#260

How to test

@james-milligan
wip
9f0763d 
Signed-off-by: James Milligan <[email protected]>
@james-milligan
add disable request logging flag
ae1c393 
Signed-off-by: James Milligan <[email protected]>
@james-milligan
removed additional file
2cc8ce2 
Signed-off-by: James Milligan <[email protected]>
@james-milligan james-milligan mentioned this pull request Nov 28, 2022
Closed
@james-milligan james-milligan changed the title feat: Noop logger feat: disable-request-logging flag Nov 28, 2022
toddbaert
toddbaert reviewed Nov 28, 2022
View reviewed changes
pkg/logger/logger.go Outdated Show resolved Hide resolved
@AlexsJones
Copy link
Member

To disable logging I have to pass --disable-request-logging that's horrible DX :(

AlexsJones
AlexsJones reviewed Nov 29, 2022
View reviewed changes
cmd/start.go Outdated Show resolved Hide resolved
pkg/logger/logger.go Outdated Show resolved Hide resolved
@james-milligan
Copy link
Contributor Author

To disable logging I have to pass --disable-request-logging that's horrible DX :(

We could have the request logging disabled by default and only enable it with the --debug flag?

@AlexsJones
Copy link
Member

To disable logging I have to pass --disable-request-logging that's horrible DX :(

We could have the request logging disabled by default and only enable it with the --debug flag?

Sounds much better to me

skyerus
skyerus reviewed Nov 29, 2022
View reviewed changes
cmd/start.go Outdated Show resolved Hide resolved
@james-milligan
Copy link
Contributor Author

Sounds much better to me

I have introduced this change

@james-milligan james-milligan changed the title feat: disable-request-logging flag feat: enable request logging via the --debug flag Nov 29, 2022
@toddbaert
Copy link
Member

To disable logging I have to pass --disable-request-logging that's horrible DX :(

We could have the request logging disabled by default and only enable it with the --debug flag?

Sounds much better to me

This makes sense to me as well.

skyerus
skyerus reviewed Nov 29, 2022
View reviewed changes
pkg/logger/logger.go Outdated Show resolved Hide resolved
pkg/logger/logger.go Outdated Show resolved Hide resolved
pkg/logger/logger.go Outdated Show resolved Hide resolved
pkg/logger/logger.go Outdated Show resolved Hide resolved
james-milligan and others added 4 commits November 29, 2022 14:18
@james-milligan @skyerus
Update pkg/logger/logger.go
2c8558d 
Co-authored-by: Skye Gill <[email protected]>
Signed-off-by: James Milligan <[email protected]>
@james-milligan @skyerus
Update pkg/logger/logger.go
fa303de 
Co-authored-by: Skye Gill <[email protected]>
Signed-off-by: James Milligan <[email protected]>
@james-milligan @skyerus
Update pkg/logger/logger.go
5646d95 
Co-authored-by: Skye Gill <[email protected]>
Signed-off-by: James Milligan <[email protected]>
@james-milligan @skyerus
Update pkg/logger/logger.go
286d51c 
Co-authored-by: Skye Gill <[email protected]>
Signed-off-by: James Milligan <[email protected]>
@AlexsJones
Copy link
Member

Is this ready for review now?

@james-milligan
Copy link
Contributor Author

Is this ready for review now?

yes :)

@beeme1mr
Copy link
Member

@james-milligan please resolve conversations that have been addressed.

@james-milligan james-milligan merged commit 11954b5 into open-feature:main Dec 2, 2022
@james-milligan james-milligan deleted the noop-logger branch December 2, 2022 09:53
@github-actions github-actions bot mentioned this pull request Dec 2, 2022
Merged
AlexsJones pushed a commit that referenced this pull request Dec 5, 2022
@github-actions
chore(main): release 0.2.7 (#215)
df7c6ee 
🤖 I have created a release *beep* *boop*
---


##
[0.2.7](v0.2.5...v0.2.7)
(2022-12-02)


### ⚠ BREAKING CHANGES

* start command flag refactor
([#222](#222))

### Features

* enable request logging via the --debug flag
([#226](#226))
([11954b5](11954b5))
* Resurrected the STATIC flag reason. Documented the caching strategy.
([#224](#224))
([5830592](5830592))
* snap ([#211](#211))
([c619844](c619844))
* start command flag refactor
([#222](#222))
([14474cc](14474cc))


### Miscellaneous Chores

* release v0.2.6
([93cfb78](93cfb78))
* release v0.2.7
([4a9f6df](4a9f6df))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skyerus skyerus skyerus left review comments

@toddbaert toddbaert toddbaert approved these changes

@AlexsJones AlexsJones AlexsJones approved these changes

@beeme1mr beeme1mr Awaiting requested review from beeme1mr

@Kavindu-Dodan Kavindu-Dodan Awaiting requested review from Kavindu-Dodan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@james-milligan @AlexsJones @toddbaert @beeme1mr @skyerus