Add logger for VSS #87
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
This is a clone of the `ldk-server` logger
tankyleo
force-pushed
the
26-01-logging
branch
from
b2bd20a to
7c84099
Compare
|
Based on #86 thought you'd want to take a look tnull given your past debugging sessions against VSS. |
| tokio::spawn(async move { | ||
| if let Err(e) = connection.await { | ||
| eprintln!("Connection error: {}", e); | ||
| warn!("Connection error: {}", e); |
There was a problem hiding this comment.
Is this recoverable? error then?
There was a problem hiding this comment.
We can recover using fn ensure_connected if this happens on one of our 10 long-lived connections to the database.
There was a problem hiding this comment.
let me know if you would rather use error here it is true that if this happens during startup we do not recover.
In case we fail during startup we do have the error!("Failed to start postgres backend"); messages at the error level.
There was a problem hiding this comment.
Yeah, I think error would be preferable in this case here, too.
There was a problem hiding this comment.
Hmm I'm not quite fully onboard yet can you explain further ? Currently I don't want to add an error log after launching the main service loop for something we can recover from.
rust/impls/Cargo.toml
Outdated
| tokio = { version = "1.38.0", default-features = false, features = ["rt", "macros"] } | ||
| native-tls = { version = "0.2.14", default-features = false } | ||
| postgres-native-tls = { version = "0.5.2", default-features = false, features = ["runtime"] } | ||
| log = "0.4.29" |
| auth | ||
| } else { | ||
| println!("No authentication method configured, all storage with the same store id will be commingled."); | ||
| warn!("No authentication method configured, all storage with the same store id will be commingled."); |
There was a problem hiding this comment.
I do start to wonder if we should only expose this behind a testing feature or cfg flag?
There was a problem hiding this comment.
Now only available via cargo run --no-default-features --features testing see below
There was a problem hiding this comment.
Could you expand why a feature is preferable to a cfg flag here? Features are meant to be user-faced, while cfg flag is really more development-oriented, and I'm leaning that NoopAuthorizer is rather the latter, i.e., should only ever be used by us / our unit tests, maybe. Everybody else should even be testing against whatever auth impl they run in production?
There was a problem hiding this comment.
Thanks for the context switched to the cfg flag below
rust/server/src/vss_service.rs
Outdated
| store: Arc<dyn KvStore>, user_token: String, request: GetObjectRequest, | ||
| ) -> Result<GetObjectResponse, VssError> { | ||
| store.get(user_token, request).await | ||
| trace!("handling GetObject request with store_id {}", request.store_id); |
There was a problem hiding this comment.
Is it worth logging truncated key values to give an indication what is being written/read?
There was a problem hiding this comment.
I want to be careful with logging what could be sensitive information. Won't the keys all be obfuscated anyway via SorableBuilder in LDK-Node ? If so an operator of vss-server wouldn't get much visibility into what is getting stored.
There was a problem hiding this comment.
I want to be careful with logging what could be sensitive information. Won't the keys all be obfuscated anyway via
SorableBuilderin LDK-Node ?
Yes, they will be obfuscated and hence logging truncated keys should be fine. It will allow to somewhat correlate with the client side if we need to debug something though.
There was a problem hiding this comment.
Sounds good I added the same logging as vss-client below, and chose to log the full keys instead of the truncated keys since these are obfuscated, and want to maintain consistent logs between vss-client and vss-server.
`NoopAuthorizer` is now only accessible via `RUSTFLAGS="--cfg noop_authorizer" cargo run --no-default-features`. We want to discourage our users from ever using `NoopAuthorizer`.
tankyleo
force-pushed
the
26-01-logging
branch
from
a595901 to
d4eae63
Compare
3 participants
This is a clone of the
ldk-serverlogger