chore: collect static log features to a separate feature #5725

MrCroxx · 2022-10-09T06:44:09Z

I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.

What's changed and what's your intention?

As titled. More discussion: #5721 .

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
All checks passed in ./risedev check (or alias, ./risedev c)

Refer to a related PR or issue link (optional)

close #5721

MrCroxx · 2022-10-09T07:06:28Z

Or we can combine static-link and static-log-level into one crate called dist later.

BugenZhao

❤️

BugenZhao · 2022-10-09T07:06:55Z

src/utils/static-log-level/src/lib.rs

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+//! This crate is used to control static log level.


To avoid forgetting to enable the static log level in release build, how about adding a compile-time check like this?

#[cfg(all(not(feature = "enabled"), not(debug_assertions)))] compile_error!("must enable `static-log-level` in release build with `--features static-log-level`")

I'd prefer moving static-log-level and static-link in a single crate. e.g. workspace-config with enable-static-link and enable-log-level feature.

BugenZhao · 2022-10-09T07:08:08Z

Cargo.lock

@@ -1969,9 +1969,9 @@ checksum = "e6d5a32815ae3f33302d95fdcb2ce17862f8c65363dcfd29360480ba1001fc9c"

 [[package]]
 name = "futures"
-version = "0.3.24"
+version = "0.3.23"


Please revert this as there's a bug in 0.3.23. 🤣 #5657 (comment)

The scaling test will fail with 0.3.23

skyzh

LGTM. Let's see if we can make things easier after we upgrade the toolchain and use workspace inheritance in the future.

skyzh · 2022-10-09T16:35:30Z

src/utils/static-log-level/src/lib.rs

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+//! This crate is used to control static log level.


I'd prefer moving static-log-level and static-link in a single crate. e.g. workspace-config with enable-static-link and enable-log-level feature.

MrCroxx · 2022-10-10T06:16:28Z

@BugenZhao What about now?

BugenZhao · 2022-10-10T06:20:37Z

LGTM. How about introducing the compile_error check?

MrCroxx · 2022-10-10T06:21:33Z

LGTM. How about introducing the compile_error check?

Oh, I forgot it. Let me add it. 😀

skyzh

Rest LGTM, good work!

skyzh · 2022-10-10T06:32:16Z

src/utils/workspace-config/src/lib.rs

@@ -13,3 +13,13 @@
 // limitations under the License.

 //! This crate includes dependencies that need to be statically-linked.
+#[cfg(all(


The magic of attribute combination 🤣

skyzh · 2022-10-10T06:32:56Z

Cargo.lock

@@ -6708,9 +6708,9 @@ checksum = "099b7128301d285f79ddd55b9a83d5e6b9e97c92e0ea0daebee7263e932de992"

 [[package]]
 name = "unicode-ident"
-version = "1.0.4"
+version = "1.0.3"


Would you please revert unrelated changes in Cargo.lock? Several crates have been downgraded.

Sure, I'm working on it~

codecov · 2022-10-10T06:59:19Z

Codecov Report

Merging #5725 (9f9cbee) into main (b0fb507) will increase coverage by 0.00%.
The diff coverage is 87.80%.

❗ Current head 9f9cbee differs from pull request most recent head 975c40d. Consider uploading reports for the commit 975c40d to get more accurate results

@@           Coverage Diff            @@
##             main    #5725    +/-   ##
========================================
  Coverage   75.07%   75.07%            
========================================
  Files         912      912            
  Lines      142660   142530   -130     
========================================
- Hits       107098   107011    -87     
+ Misses      35562    35519    -43

Flag	Coverage Δ
rust	`75.07% <87.80%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/meta/src/rpc/server.rs	`80.21% <ø> (-0.21%)`	⬇️
src/meta/src/stream/source_manager.rs	`31.36% <ø> (+1.12%)`	⬆️
src/meta/src/stream/stream_manager.rs	`71.15% <ø> (-0.07%)`	⬇️
src/stream/src/from_proto/merge.rs	`0.00% <0.00%> (ø)`
src/stream/src/task/stream_manager.rs	`2.75% <0.00%> (+<0.01%)`	⬆️
src/utils/workspace-config/src/lib.rs	`100.00% <ø> (ø)`
src/meta/src/hummock/manager/mod.rs	`79.09% <50.00%> (-0.16%)`	⬇️
src/meta/src/manager/id.rs	`93.36% <92.59%> (-0.12%)`	⬇️
src/meta/src/hummock/compaction_group/manager.rs	`90.38% <95.23%> (+0.53%)`	⬆️
src/meta/src/manager/cluster.rs	`77.30% <100.00%> (ø)`
... and 13 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

StrikeW · 2022-10-12T03:27:19Z

It seems that the DEBUG log of aws_config::meta::credentials::lazy_caching in the release build doesn't suppress after this PR. Have you ever try to run the cluster and see the log output? cc @MrCroxx

./risedev configure to set the release build then start the cluster with ./risedev d full (use aws-s3 instead of minio). And debug log of aws_config still shows up.
compactor log:

2022-10-12T03:23:15.535807Z DEBUG send_operation:provide_credentials: aws_config::meta::credentials::lazy_caching: loaded credentials from cache operation="PutObject" service="s3" provider=default_chain
2022-10-12T03:23:15.784546Z DEBUG send_operation:provide_credentials: aws_config::meta::credentials::lazy_caching: loaded credentials from cache operation="PutObject" service="s3" provider=default_chain
2022-10-12T03:23:15.955515Z DEBUG send_operation:provide_credentials: aws_config::meta::credentials::lazy_caching: loaded credentials from cache operation="PutObject" service="s3" provider=default_chain
2022-10-12T03:23:16.341963Z  INFO risingwave_storage::hummock::compactor: Finished compaction task in 1126.990332ms:
Compaction task id: 9, target level: 0
Compaction watermark: 3165976256512000
Compaction target_file_size: 33554432
Compaction # splits: 1
Compaction task status: 2
Compaction Sstables structure:
Level 0 ["[id: 50, 9114KB]"]
...

chore: collect static log features to a separate feature

ad7e57b

MrCroxx mentioned this pull request Oct 9, 2022

dev: collect all tracing *_max_level_* features into a separate feature #5721

Closed

github-actions bot added the type/chore label Oct 9, 2022

BugenZhao reviewed Oct 9, 2022

View reviewed changes

BugenZhao requested a review from skyzh October 9, 2022 07:08

Merge branch 'main' into xx/log-level

c1fe887

skyzh approved these changes Oct 9, 2022

View reviewed changes

MrCroxx added 2 commits October 10, 2022 13:31

Merge branch 'main' into xx/log-level

7335b9e

combine workspace-config crate

bdca057

MrCroxx marked this pull request as ready for review October 10, 2022 06:15

MrCroxx added 2 commits October 10, 2022 06:26

add compile error for release

58cc36e

fix error message

3377589

BugenZhao approved these changes Oct 10, 2022

View reviewed changes

skyzh approved these changes Oct 10, 2022

View reviewed changes

clean Cargo.lock

9f9cbee

MrCroxx self-assigned this Oct 10, 2022

MrCroxx added the mergify/can-merge Indicates that the PR can be added to the merge queue label Oct 10, 2022

Merge branch 'main' into xx/log-level

975c40d

mergify bot merged commit 7678927 into main Oct 10, 2022

mergify bot deleted the xx/log-level branch October 10, 2022 07:16

MrCroxx mentioned this pull request Oct 13, 2022

bug: risedev build in release mode compile error #5829

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: collect static log features to a separate feature #5725

chore: collect static log features to a separate feature #5725

MrCroxx commented Oct 9, 2022

MrCroxx commented Oct 9, 2022

BugenZhao left a comment

BugenZhao Oct 9, 2022

skyzh Oct 9, 2022

BugenZhao Oct 9, 2022

BugenZhao Oct 10, 2022

skyzh left a comment

skyzh Oct 9, 2022

MrCroxx commented Oct 10, 2022

BugenZhao commented Oct 10, 2022

MrCroxx commented Oct 10, 2022

skyzh left a comment

skyzh Oct 10, 2022

MrCroxx Oct 10, 2022

skyzh Oct 10, 2022

MrCroxx Oct 10, 2022

codecov bot commented Oct 10, 2022 •

edited

StrikeW commented Oct 12, 2022 •

edited

chore: collect static log features to a separate feature #5725

chore: collect static log features to a separate feature #5725

Conversation

MrCroxx commented Oct 9, 2022

What's changed and what's your intention?

Checklist

Refer to a related PR or issue link (optional)

MrCroxx commented Oct 9, 2022

BugenZhao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skyzh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrCroxx commented Oct 10, 2022

BugenZhao commented Oct 10, 2022

MrCroxx commented Oct 10, 2022

skyzh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 10, 2022 • edited

Codecov Report

StrikeW commented Oct 12, 2022 • edited

codecov bot commented Oct 10, 2022 •

edited

StrikeW commented Oct 12, 2022 •

edited