Add logger for VSS #87
Conversation
|
👋 I see @tnull was un-assigned. |
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.
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?
| 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.
No only accessible via `cargo run --no-default-features --features testing`
3 participants
This is a clone of the
ldk-serverlogger