GH-35260: [C++][Python][R] Allow users to adjust S3 log level by environment variable #38267

amoeba · 2023-10-14T01:12:36Z

Rationale for this change

It's useful when troubleshooting issues with Arrow's S3 filesystem implementation to raise the log level. Currently, this can only be done in C++ and Python, but not from R. In addition, the log level can only be set during S3 initialization and not directly so the user has to introduce explicit S3 initialization code to turn on logging and must make sure this code is called before S3 initialization.

While discussing exposing control of log level to R, we realized that allowing the log level to be controlled by environment variable may be more intuitive and useful and would just be a good addition for C++, Python, and R.

What changes are included in this PR?

A new environment variable AWS_S3_LOG_LEVEL with documentation for controlling S3 log level
Updated documentation for C++, Python, and R
A new InitializeS3() as a quality-of-life thing for C++ users. Feel free to ask me to remove this.

No changes are needed directly for Python and R because these implementation uses the internal implicit initializer EnsureS3Initialized rather than the explicit form, InitializeS3. And it's the behavior of the EnsureS3Initialized routine that's changed here.

Are these changes tested?

Yes. I added a unit test for the new GetS3LogLevelFromEnvOrDefault and tested from Python and R manually. I didn't add a test to make sure the underlying AwsInstance gets set up correctly because it looked like it would require a refactor and didn't seem worth it.

Are there any user-facing changes?

Yes. A new way to turn on logging for S3 and matching docs in C++, Python, and R.

Closes: [R] Allow users to adjust AWS S3 log level from R #35260

All implementing done in C++, documentation added for C++, Python, and R

These are already exported and IMO part of the public API

amoeba · 2023-10-14T01:15:49Z

cpp/src/arrow/filesystem/s3fs.cc

@@ -3016,5 +3023,35 @@ Result<std::string> ResolveS3BucketRegion(const std::string& bucket) {
  return resolver->ResolveRegion(bucket);
 }

+S3LogLevel GetS3LogLevelFromEnvOrDefault() {


Feel free to nit about this function name.

I like arrow::fs::S3GlobalOptions::Defaults().

docs/source/cpp/env_vars.rst

r/R/filesystem.R

thisisnic · 2023-10-14T07:50:02Z

From an R perspective, looks great! What do you think to the idea of adding a new H2-level heading to the bottom of the "debugging" doc in the R developer docs, briefly mentioning this? Doesn't need to be in this PR!

Co-authored-by: Nic Crane <thisisnic@gmail.com>

amoeba · 2023-10-14T17:56:23Z

Great idea @thisisnic, filed as #38270. Thanks.

cpp/src/arrow/filesystem/s3fs.h

kou · 2023-10-14T20:55:54Z

cpp/src/arrow/filesystem/s3fs.cc

@@ -3016,5 +3023,35 @@ Result<std::string> ResolveS3BucketRegion(const std::string& bucket) {
  return resolver->ResolveRegion(bucket);
 }

+S3LogLevel GetS3LogLevelFromEnvOrDefault() {


I like arrow::fs::S3GlobalOptions::Defaults().

cpp/src/arrow/filesystem/s3fs_test.cc

docs/source/cpp/env_vars.rst

docs/source/python/filesystems.rst

Co-authored-by: Sutou Kouhei <kou@cozmixng.org>

amoeba · 2023-10-16T19:47:30Z

Thanks again @kou, sorry this PR turned out to be a bit messy. I've addressed all of your comments and accepted all suggestions.

cpp/src/arrow/filesystem/s3fs.cc

docs/source/cpp/api/filesystem.rst

kou

+1

amoeba · 2023-10-17T17:13:36Z

Thanks for the help getting this tidied up @kou!

conbench-apache-arrow · 2023-10-20T01:16:53Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 1e9f224.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them.

…y environment variable (apache#38267) ### Rationale for this change It's useful when troubleshooting issues with Arrow's S3 filesystem implementation to raise the log level. Currently, this can only be done in C++ and Python, but not from R. In addition, the log level can only be set during S3 initialization and not directly so the user has to introduce explicit S3 initialization code to turn on logging and must make sure this code is called before S3 initialization. While discussing exposing control of log level to R, we realized that allowing the log level to be controlled by environment variable may be more intuitive and useful and would just be a good addition for C++, Python, and R. ### What changes are included in this PR? - A new environment variable `AWS_S3_LOG_LEVEL` with documentation for controlling S3 log level - Updated documentation for C++, Python, and R - A new `InitializeS3()` as a quality-of-life thing for C++ users. Feel free to ask me to remove this. No changes are needed directly for Python and R because these implementation uses the internal implicit initializer `EnsureS3Initialized` rather than the explicit form, `InitializeS3`. And it's the behavior of the `EnsureS3Initialized` routine that's changed here. ### Are these changes tested? Yes. I added a unit test for the new `GetS3LogLevelFromEnvOrDefault` and tested from Python and R manually. I didn't add a test to make sure the underlying `AwsInstance` gets set up correctly because it looked like it would require a refactor and didn't seem worth it. ### Are there any user-facing changes? Yes. A new way to turn on logging for S3 and matching docs in C++, Python, and R. * Closes: apache#35260 Lead-authored-by: Bryce Mecum <petridish@gmail.com> Co-authored-by: Sutou Kouhei <kou@cozmixng.org> Co-authored-by: Nic Crane <thisisnic@gmail.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

amoeba added 2 commits October 13, 2023 16:44

Allow setting log level in S3 by environment variable

486fc6b

All implementing done in C++, documentation added for C++, Python, and R

Add InitializeS3 functions to C++ API docs

b4167a8

These are already exported and IMO part of the public API

amoeba requested review from paleolimbot and thisisnic as code owners October 14, 2023 01:12

github-actions bot added Component: R Component: C++ Component: Documentation awaiting review Awaiting review labels Oct 14, 2023

amoeba commented Oct 14, 2023

View reviewed changes

docs/source/cpp/env_vars.rst Show resolved Hide resolved

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Oct 14, 2023

Export GetS3LogLevelFromEnvOrDefault

b88328b

thisisnic reviewed Oct 14, 2023

View reviewed changes

r/R/filesystem.R Outdated Show resolved Hide resolved

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Oct 14, 2023

Update r/R/filesystem.R

c2f0d13

Co-authored-by: Nic Crane <thisisnic@gmail.com>

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Oct 14, 2023

amoeba mentioned this pull request Oct 14, 2023

[R][Docs] Add section on debugging S3 in the R developer docs #38270

Open

kou reviewed Oct 14, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Oct 14, 2023

Update cpp/src/arrow/filesystem/s3fs.h

5773902

Co-authored-by: Sutou Kouhei <kou@cozmixng.org>

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Oct 14, 2023

amoeba and others added 2 commits October 14, 2023 13:48

Update docs/source/cpp/env_vars.rst

e4407a4

Co-authored-by: Sutou Kouhei <kou@cozmixng.org>

Update cpp/src/arrow/filesystem/s3fs_test.cc

508b733

Co-authored-by: Sutou Kouhei <kou@cozmixng.org>

github-actions bot added the awaiting change review Awaiting change review label Oct 16, 2023

amoeba and others added 4 commits October 16, 2023 11:25

Update cpp/src/arrow/filesystem/s3fs_test.cc

6234073

Co-authored-by: Sutou Kouhei <kou@cozmixng.org>

Update cpp/src/arrow/filesystem/s3fs_test.cc

5ad279d

Co-authored-by: Sutou Kouhei <kou@cozmixng.org>

Update cpp/src/arrow/filesystem/s3fs_test.cc

0259cca

Co-authored-by: Sutou Kouhei <kou@cozmixng.org>

Clean up unused methods

e70deca

lint

efa32f7

kou reviewed Oct 17, 2023

View reviewed changes

cpp/src/arrow/filesystem/s3fs.cc Outdated Show resolved Hide resolved

Use auto

5198758

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Oct 17, 2023

kou reviewed Oct 17, 2023

View reviewed changes

docs/source/cpp/api/filesystem.rst Outdated Show resolved Hide resolved

Remove nonexistent entry

340ec67

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes labels Oct 17, 2023

kou approved these changes Oct 17, 2023

View reviewed changes

kou merged commit 1e9f224 into apache:main Oct 17, 2023
35 checks passed

kou removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Oct 17, 2023

github-actions bot added the awaiting merge Awaiting merge label Oct 17, 2023

This was referenced Oct 17, 2023

[R] read_parquet performs to slow #38032

Open

[C++][Python][R] Add environment variable for controlling S3 debug level #36115

Closed

justinvyu mentioned this pull request Nov 17, 2023

[Train] AWS Error NETWORK_CONNECTION during HeadObject operation: curlCode: 43 during checkpoint reporting ray-project/ray#41137

Open

amoeba mentioned this pull request Jan 17, 2024

[Website] Version 15.0.0 blog post apache/arrow-site#464

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-35260: [C++][Python][R] Allow users to adjust S3 log level by environment variable #38267

GH-35260: [C++][Python][R] Allow users to adjust S3 log level by environment variable #38267

amoeba commented Oct 14, 2023 •

edited by github-actions bot

Loading

amoeba Oct 14, 2023

kou Oct 14, 2023

thisisnic commented Oct 14, 2023

amoeba commented Oct 14, 2023

kou Oct 14, 2023

amoeba commented Oct 16, 2023

kou left a comment

amoeba commented Oct 17, 2023

conbench-apache-arrow bot commented Oct 20, 2023

GH-35260: [C++][Python][R] Allow users to adjust S3 log level by environment variable #38267

GH-35260: [C++][Python][R] Allow users to adjust S3 log level by environment variable #38267

Conversation

amoeba commented Oct 14, 2023 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

amoeba Oct 14, 2023

Choose a reason for hiding this comment

kou Oct 14, 2023

Choose a reason for hiding this comment

thisisnic commented Oct 14, 2023

amoeba commented Oct 14, 2023

kou Oct 14, 2023

Choose a reason for hiding this comment

amoeba commented Oct 16, 2023

kou left a comment

Choose a reason for hiding this comment

amoeba commented Oct 17, 2023

conbench-apache-arrow bot commented Oct 20, 2023

amoeba commented Oct 14, 2023 •

edited by github-actions bot

Loading