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

Suppress tusd logs #3552

Merged
merged 2 commits into from
Dec 12, 2022
Merged

Suppress tusd logs #3552

merged 2 commits into from
Dec 12, 2022

Conversation

kobergj
Copy link
Contributor

@kobergj kobergj commented Dec 12, 2022

fixes owncloud/ocis#4803 by passing a Logger to the tusd package

@update-docs
Copy link

update-docs bot commented Dec 12, 2022

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

Signed-off-by: jkoberg <[email protected]>
Copy link
Member

@micbar micbar left a comment

Choose a reason for hiding this comment

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

Good catch.

@butonic
Copy link
Contributor

butonic commented Dec 12, 2022

well, yeah, this disables the log ... but there is no way to reenable it. I guess we need to invest in upstream patches that fetch the logger from the context. or maybe fetching the logger from the context is not a good pattern? Then again, I don't see a good way of changing the log level for different users ... maybe using logging for user specific logs is the wrong approach. Maybe that is better done with tracing and span attributes.

That should in theory allow us to pass a logger to the handlers, like the tus handler, wrap it with a trace middleware and have the best of both worlds?

Not part of this PR, though ...

@butonic butonic merged commit 894b879 into cs3org:edge Dec 12, 2022
@kobergj kobergj deleted the SuppressTusdLog branch December 13, 2022 08:20
@kobergj
Copy link
Contributor Author

kobergj commented Dec 13, 2022

Why is there no way to reenable it? We pass a custom logger. I expect it to log through the logger we provided. Otherwise this is a upstream bug (either in basic log or zerolog package).

@@ -99,6 +100,7 @@ func (m *manager) Handler(fs storage.FS) (http.Handler, error) {
config := tusd.Config{
StoreComposer: composer,
NotifyCompleteUploads: true,
Logger: log.New(appctx.GetLogger(context.Background()), "", 0),
Copy link
Member

Choose a reason for hiding this comment

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

hmm, now that the discussion is on, let me throw in my 2 cents. Can we pass in the active context instead of creating it on the fly because the active context holds the requestID.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no active context. This happens on service init

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.

3 participants