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

Remove compatibility layer in tracing #4535

Merged
merged 1 commit into from
Dec 15, 2023
Merged

Conversation

aoudiamoncef
Copy link
Contributor

  • document all added functions
  • try in sandbox /simulation/labnet
    • if part of node-launch, checked using the resync_check flag
  • unit tests on the added/changed features
    • make tests compile
    • make tests pass
  • add logs allowing easy debugging in case the changes caused problems
  • if the API has changed, update the API specification

closes #4478

Leo-Besancon
Leo-Besancon previously approved these changes Nov 14, 2023
@AurelienFT
Copy link
Contributor

A question about this changes, is it still possible to filter the logs with RUST_LOG standard log variable in this PR ?

@aoudiamoncef
Copy link
Contributor Author

I'll check this before merging

IMHO, we should work on an easy way to enable and disable dependencies logs inside Massa. I did this with @modship to debug an issue in jsonrpsee.

@aoudiamoncef
Copy link
Contributor Author

@AurelienFT could you test the behavior on main branch without this changes ? I get a weird result, I would like to get a confirmation.

@Leo-Besancon Leo-Besancon changed the base branch from main to buildnet_wip November 15, 2023 07:04
@AurelienFT
Copy link
Contributor

@AurelienFT could you test the behavior on main branch without this changes ? I get a weird result, I would like to get a confirmation.

When launching the node, we override the default value that could be in the env variable with our config.toml but in case of the test for example I use this standard variable (

tracing_subscriber::fmt::layer().with_filter(EnvFilter::from_default_env());
) and so in the test that use the new framework we can choose level of print we want when we run our tests using the rust standard way.

@aoudiamoncef
Copy link
Contributor Author

@AurelienFT can you confirm that It's working as expected in main branch ? (launch in debug with sandbox)

It's cool to choose, if we could inject a list of logger to enable with an env variable, It'll be awesome

@AurelienFT
Copy link
Contributor

I just answered you above. The behaviour on the node will work as we are not using the env variable but a config file. In the tests it's not the case. I just want to be sure that your changes doesn't prevents us to see the logs in the tests.

@aoudiamoncef
Copy link
Contributor Author

@AurelienFT after testing on buildnet_wip branch, It works

@AurelienFT
Copy link
Contributor

And on this branch it works also?

@Leo-Besancon Leo-Besancon changed the base branch from buildnet_wip to pre_mainnet November 22, 2023 08:36
Base automatically changed from pre_mainnet to main November 28, 2023 08:34
@Leo-Besancon Leo-Besancon dismissed their stale review November 28, 2023 08:34

The base branch was changed.

@Leo-Besancon Leo-Besancon changed the base branch from main to pre_mainnet_wip November 28, 2023 08:40
@Leo-Besancon Leo-Besancon changed the base branch from pre_mainnet_wip to test_27_6_wip December 15, 2023 08:08
Copy link
Collaborator

@Leo-Besancon Leo-Besancon left a comment

Choose a reason for hiding this comment

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

I confirm that RUST_LOG var env is taken into account both with tracing:: and tracing::log::

Copy link

codecov bot commented Dec 15, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (9a4f384) 71.78% compared to head (dd92b06) 71.78%.

Files Patch % Lines
massa-protocol-worker/src/connectivity.rs 0.00% 2 Missing ⚠️
...rotocol-worker/src/handlers/peer_handler/tester.rs 50.00% 1 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff               @@
##           test_27_6_wip    #4535   +/-   ##
==============================================
  Coverage          71.78%   71.78%           
==============================================
  Files                291      291           
  Lines              51642    51642           
==============================================
  Hits               37069    37069           
  Misses             14573    14573           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@aoudiamoncef aoudiamoncef merged commit 0246173 into test_27_6_wip Dec 15, 2023
12 checks passed
@aoudiamoncef aoudiamoncef deleted the hotfix/tracing-dep branch December 15, 2023 09:18
damip added a commit that referenced this pull request Dec 15, 2023
* Update addresses and public keys

* Add more info to double staking protection's panic message

* fmt

* update version and genesis

* Update version and CI

* Update Cargo.lock

* add community charter

* Add serde json tests (#4579)

* chore: typo fix (#4589)

* Update to wasmer 4.2.4 (#4581)

Co-authored-by: sydhds <[email protected]>

* typo fix

* typo fix

* typo fix

* typo fix

* typo fix

---------

Co-authored-by: Sydhds <[email protected]>
Co-authored-by: sydhds <[email protected]>

* ci: add checksum + tested with ACT (#4590)

* Upgrade-to-latest-toolchain (#4585)

Co-authored-by: Leo-Besancon <[email protected]>

* Update massa-sc-runtime

Signed-off-by: Jean-François <[email protected]>

* Cargo update

* Remove compatibility layer in tracing (#4535)

* Add deferred_credits initial file for test

* Update Genesis and CI

* Push back Genesis

* Same

---------

Signed-off-by: Jean-François <[email protected]>
Co-authored-by: Damir Vodenicarevic <[email protected]>
Co-authored-by: Moncef AOUDIA <[email protected]>
Co-authored-by: Ursulafe <[email protected]>
Co-authored-by: Sydhds <[email protected]>
Co-authored-by: sydhds <[email protected]>
Co-authored-by: Jean-François <[email protected]>
Leo-Besancon added a commit that referenced this pull request Dec 15, 2023
* Update addresses and public keys

* Add more info to double staking protection's panic message

* fmt

* update version and genesis

* Update version and CI

* Update Cargo.lock

* add community charter

* Add serde json tests (#4579)

* chore: typo fix (#4589)

* Update to wasmer 4.2.4 (#4581)

Co-authored-by: sydhds <[email protected]>

* typo fix

* typo fix

* typo fix

* typo fix

* typo fix

---------

Co-authored-by: Sydhds <[email protected]>
Co-authored-by: sydhds <[email protected]>

* ci: add checksum + tested with ACT (#4590)

* Upgrade-to-latest-toolchain (#4585)

Co-authored-by: Leo-Besancon <[email protected]>

* Update massa-sc-runtime

Signed-off-by: Jean-François <[email protected]>

* Cargo update

* Remove compatibility layer in tracing (#4535)

* Add deferred_credits initial file for test

* Update Genesis and CI

* Push back Genesis

* Same

---------

Signed-off-by: Jean-François <[email protected]>
Co-authored-by: Damir Vodenicarevic <[email protected]>
Co-authored-by: Moncef AOUDIA <[email protected]>
Co-authored-by: Ursulafe <[email protected]>
Co-authored-by: Sydhds <[email protected]>
Co-authored-by: sydhds <[email protected]>
Co-authored-by: Jean-François <[email protected]>
sydhds added a commit that referenced this pull request Dec 20, 2023
* Update addresses and public keys

* Add more info to double staking protection's panic message

* fmt

* update version and genesis

* Update version and CI

* Update Cargo.lock

* add community charter

* Add serde json tests (#4579)

* chore: typo fix (#4589)

* Update to wasmer 4.2.4 (#4581)

Co-authored-by: sydhds <[email protected]>

* typo fix

* typo fix

* typo fix

* typo fix

* typo fix

---------

Co-authored-by: Sydhds <[email protected]>
Co-authored-by: sydhds <[email protected]>

* ci: add checksum + tested with ACT (#4590)

* Upgrade-to-latest-toolchain (#4585)

Co-authored-by: Leo-Besancon <[email protected]>

* Update massa-sc-runtime

Signed-off-by: Jean-François <[email protected]>

* Cargo update

* Remove compatibility layer in tracing (#4535)

* Add deferred_credits initial file for test

* Update Genesis and CI

* Push back Genesis

* Same

---------

Signed-off-by: Jean-François <[email protected]>
Co-authored-by: Damir Vodenicarevic <[email protected]>
Co-authored-by: Moncef AOUDIA <[email protected]>
Co-authored-by: Ursulafe <[email protected]>
Co-authored-by: Sydhds <[email protected]>
Co-authored-by: sydhds <[email protected]>
Co-authored-by: Jean-François <[email protected]>
sydhds added a commit that referenced this pull request Dec 20, 2023
* Update addresses and public keys

* Add more info to double staking protection's panic message

* fmt

* update version and genesis

* Update version and CI

* Update Cargo.lock

* add community charter

* Add serde json tests (#4579)

* chore: typo fix (#4589)

* Update to wasmer 4.2.4 (#4581)

Co-authored-by: sydhds <[email protected]>

* typo fix

* typo fix

* typo fix

* typo fix

* typo fix

---------

Co-authored-by: Sydhds <[email protected]>
Co-authored-by: sydhds <[email protected]>

* ci: add checksum + tested with ACT (#4590)

* Upgrade-to-latest-toolchain (#4585)

Co-authored-by: Leo-Besancon <[email protected]>

* Update massa-sc-runtime

Signed-off-by: Jean-François <[email protected]>

* Cargo update

* Remove compatibility layer in tracing (#4535)

* Add deferred_credits initial file for test

* Update Genesis and CI

* Push back Genesis

* Same

---------

Signed-off-by: Jean-François <[email protected]>
Co-authored-by: Damir Vodenicarevic <[email protected]>
Co-authored-by: Moncef AOUDIA <[email protected]>
Co-authored-by: Ursulafe <[email protected]>
Co-authored-by: Sydhds <[email protected]>
Co-authored-by: sydhds <[email protected]>
Co-authored-by: Jean-François <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request Dec 22, 2023
* Add initial chain id impl

* Add chain id to jsonrpc, grpc public & grpc private api

* Add massa-client features sandbox

* Add chain id SC unit test

* Fix unit test + add dummy gas costs

* Update massa-proto-rs dependency

* Update massa-sc-runtime dependency

* Add unit test allowing easy update of massa docs

* Use chain id through config instead of constants directly

* Use chain id everywhere (round 1) (#4594)

* Use chain id everywhere (round 1)

* Use chain id in massa client

---------

Co-authored-by: sydhds <[email protected]>

* Remove confusing chain id

* Fix some unit tests

* Test 27 6 wip (#4582)

* Update addresses and public keys

* Add more info to double staking protection's panic message

* fmt

* update version and genesis

* Update version and CI

* Update Cargo.lock

* add community charter

* Add serde json tests (#4579)

* chore: typo fix (#4589)

* Update to wasmer 4.2.4 (#4581)

Co-authored-by: sydhds <[email protected]>

* typo fix

* typo fix

* typo fix

* typo fix

* typo fix

---------

Co-authored-by: Sydhds <[email protected]>
Co-authored-by: sydhds <[email protected]>

* ci: add checksum + tested with ACT (#4590)

* Upgrade-to-latest-toolchain (#4585)

Co-authored-by: Leo-Besancon <[email protected]>

* Update massa-sc-runtime

Signed-off-by: Jean-François <[email protected]>

* Cargo update

* Remove compatibility layer in tracing (#4535)

* Add deferred_credits initial file for test

* Update Genesis and CI

* Push back Genesis

* Same

---------

Signed-off-by: Jean-François <[email protected]>
Co-authored-by: Damir Vodenicarevic <[email protected]>
Co-authored-by: Moncef AOUDIA <[email protected]>
Co-authored-by: Ursulafe <[email protected]>
Co-authored-by: Sydhds <[email protected]>
Co-authored-by: sydhds <[email protected]>
Co-authored-by: Jean-François <[email protected]>

* Add initial chain id impl

* Use chain id everywhere (round 1) (#4594)

* Use chain id everywhere (round 1)

* Use chain id in massa client

---------

Co-authored-by: sydhds <[email protected]>

* Fix unit tests (final round)

* Minor fix

* Revert some nasty insert from bad rebase

* Auto gen chain id from VERSION

* Cargo clippy && fmt && doc pass

* Update massa-sc-runtime dependency

---------

Signed-off-by: Jean-François <[email protected]>
Co-authored-by: sydhds <[email protected]>
Co-authored-by: Leo-Besancon <[email protected]>
Co-authored-by: Damir Vodenicarevic <[email protected]>
Co-authored-by: Moncef AOUDIA <[email protected]>
Co-authored-by: Ursulafe <[email protected]>
Co-authored-by: Jean-François <[email protected]>
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.

Replace compatibility layer in tracing
3 participants