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

Replace opentelemetry-jaeger with opentelemetry-otlp as a more flexible alternative. #7563

Merged
merged 28 commits into from
Sep 7, 2022

Conversation

nikurt
Copy link
Contributor

@nikurt nikurt commented Sep 6, 2022

@nikurt nikurt requested a review from a team as a code owner September 6, 2022 13:29
@nikurt nikurt requested a review from mina86 September 6, 2022 13:29
@matklad matklad requested a review from nagisa September 6, 2022 13:30
]

[[package]]
name = "prost-build"
Copy link
Contributor

Choose a reason for hiding this comment

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

Huh, it makes no sense I think that we pull code generation here... I would expect the rust code to be pre-generated...

Will ask tonic folks about this.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mina86 mina86 removed their request for review September 6, 2022 15:13
@mina86
Copy link
Contributor

mina86 commented Sep 6, 2022

I’m assuming that I’m not needed here any more. ;)

Copy link
Collaborator

@nagisa nagisa left a comment

Choose a reason for hiding this comment

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

I can vouch for using otlp. I was actually under an impression that we were already using otlp 🤔…

mina86 and others added 18 commits September 7, 2022 20:06
This consolidates three unsafe blocks into a single location.
This is a pure refactor, including two main refactors:

- replace `TrieCacheFactory` with `TrieConfig`
- move config constants into `TrieConfig`

The motivation is to facilitate wider configuration and changes to the
config format in follow-up PRs.

See also #7564.
This PR replaces an unmaintained fork of secp256k1 to the latest version of the original library that already have all required features with a more clear API. Also this will solve compilation issues with llvm 13.* tools on MacOS like the next: rust-lang/rust-bindgen#1834
Firstly, change is_on_current_chain method to return boolean status
rather than error if block is not on current chain.  The method is
used in two places: in one it’s only used as a boolean predicate so
returned error is irrelevant; in the other one the caller performs
another check and duplicates code for creating the error.  Changing
is_on_current_chain return bool simplifies both call sites.

Secondly, rewrite check_block_final_and_canonical to accept a list of
headers rather than operating on single block hash.  (With that also
change ‘block’ in name into ‘blocks’).  This has a few motivations:
i) it makes last final block header being fetched only once, ii) since
caller needs access to the block header this makes it so that header
is fetched only once and iii) moving fetching of the headers to the
caller will actually help slightly in upcoming efforts for cold
storage.
The database corruption issue when disk is running out of space has
been fixed upstream in facebook/rocksdb#6316.
There’s no reason for us to carry our own hacky workaround.
#7574)

This does three relate things:

1. When opening the database in read-only mode, StoreOpener now checks
   whether database version is the one node is built for.  If it
   isn’t, opening fails.  This is because we cannot perform migrations
   in read-only mode so there’s nothing we can do with the database.

2. When creating the database, initialise it by setting its version to
   the one node was built for.  This moves the code from nearcore.

3. Introduce StoreOpener::create which refuses to open already
   existing database.  Motivation here is mostly to remove database
   existence check from recompress_storage and thus get rid of the
   check_if_exists method to shrink StoreOpener API.
This is a great help in debugging, and I don't think there's much
downside to doing that? Capturing a backtrace is somewhat costly,
especially on windows, but given that we run with panic=abort, that's
bound to be a pretty cold path anyways!

To disable backtraces, set `RUST_BACKTRACE=0` env variable.

Test Plan
---------

Manually check that both default&override work
Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>
Now it is straightforward to see how messages are read from the network socket.
* added tests for connection pool

* Update chain/network/src/broadcast/mod.rs

Co-authored-by: Simonas Kazlauskas <git@kazlauskas.me>

* applied comments

* compilation fix

Co-authored-by: Simonas Kazlauskas <git@kazlauskas.me>
Co-authored-by: near-bulldozer[bot] <73298989+near-bulldozer[bot]@users.noreply.github.com>
cc: @matklad 

```
% cargo bench -p near-o11y
   Compiling near-o11y v0.0.0 (/storage/code/nearcore-master/core/o11y)
    Finished bench [optimized] target(s) in 1.05s
     Running unittests src/lib.rs (target/release/deps/near_o11y-ccbb448e66a5a4d9)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running benches/metrics.rs (target/release/deps/metrics-d531efabdee84e90)

running 3 tests
test inc_counter_vec_cached            ... bench:          21 ns/iter (+/- 1)
test inc_counter_vec_cached_str        ... bench:         183 ns/iter (+/- 1)
test inc_counter_vec_with_label_values ... bench:         506 ns/iter (+/- 10)

test result: ok. 0 passed; 0 failed; 0 ignored; 3 measured
```
…re (#7569)

# account_id_in_function_call_permission

This feature controls [the check](https://github.com/near/nearcore/blob/b315192e84d388671316deaa3a17ece9d0565fd1/runtime/runtime/src/verifier.rs#L400-L405) which enforces that account id in function call permission is indeed a valid account id. Before, any string could have been used there. The primary motivation is robustness -- by restricting permissions to only valid account ids, we don't have to deal with potentially arbitrary long strings in storage. 

# Context

- Implementation: #7139

# Testing and QA

We have basic and upgradability test [here](https://github.com/near/nearcore/blob/master/integration-tests/src/tests/client/features/account_id_in_function_call_permission.rs#L18). This PR also adds a test for an extra edge case with overly long account id. This feature have been running on betanet for couple of months without problems. 

# Checklist
- [x] Link to nightly nayduck run: https://nayduck.near.org/#/run/2667
- [x] Update CHANGELOG.md to include this protocol feature in the `Unreleased` section.
Now it is straightforward to see how messages are read from the network socket.
@near-bulldozer near-bulldozer bot merged commit 2db3152 into master Sep 7, 2022
@near-bulldozer near-bulldozer bot deleted the nikurt-otlp branch September 7, 2022 19:07
nikurt added a commit that referenced this pull request Nov 9, 2022
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.

9 participants