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

objstore/azure: Disable Azure blob exception logging #3331

Merged
merged 1 commit into from
Oct 19, 2020
Merged

objstore/azure: Disable Azure blob exception logging #3331

merged 1 commit into from
Oct 19, 2020

Conversation

pharaujo
Copy link
Contributor

@pharaujo pharaujo commented Oct 16, 2020

Ever since the introduction of the deletion marker, Thanos compact has been filling the logs with Azure blob storage exceptions. As discussed in #2565, the error logging should be handled by Thanos itself, so this patch turns off standard logging from the Azure library.

However, due to the way logging is set up in the library, it will always log errors to syslog, regardless of how ShouldLog is configured. As such, and until that can be a configurable behavior, care should be taken to handle that from the syslog side as well.

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

Disabled exception logging from the Azure Blob storage module

Verification

Compiled locally and ran thanos compact against an Azure blob storage container.

@pharaujo pharaujo marked this pull request as ready for review October 16, 2020 14:22
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

LGTM, just would be nice to comment on surprises. Value -1 turning off something is super weird (:

Thanks!

pkg/objstore/azure/helpers.go Show resolved Hide resolved
Ever since the introduction of the deletion marker, Thanos compact has
been filling the logs with Azure blob storage exceptions. As discussed
in #2565, the error logging should be handled by Thanos itself, so this
patch turns off standard logging from the Azure library.

However, [due to the way logging is set up in the library][1], it will
always log errors to syslog, regardless of how `ShouldLog` is
configured. As such, and until that can be a configurable behavior, care
should be taken to handle that from the syslog side as well.

[1]: https://github.com/Azure/azure-storage-blob-go/blob/48358e1de5110852097ebbc11c53581d64d47300/azblob/zc_policy_request_log.go#L100-L102

Signed-off-by: Pedro Araujo <phcrva@gmail.com>
@pharaujo
Copy link
Contributor Author

LGTM, just would be nice to comment on surprises. Value -1 turning off something is super weird (:

Yeah, I agree. The whole logging method is super weird, it forces logging even if you explicitly request for it to not log. I plan on at least opening an issue for that on their end.

@pharaujo
Copy link
Contributor Author

@bwplotka I didn't change anything since the previous commit that would cause the failed test, only added a comment as discussed. How should I proceed?

Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

LGTM

@kakkoyun kakkoyun merged commit 7ec45ef into thanos-io:master Oct 19, 2020
@pharaujo pharaujo deleted the issue-2565 branch October 19, 2020 08:47
kakkoyun pushed a commit that referenced this pull request Oct 20, 2020
As a follow-up to #3331, this disables Azure exception logging to
syslog.

Signed-off-by: Pedro Araujo <phcrva@gmail.com>
@pharaujo pharaujo changed the title Disable Azure blob exception logging objstore/azure: Disable Azure blob exception logging Oct 22, 2020
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