This updates our logging framework from legacy freetext strings using
the `log` package to structured log entries using `log/slog`. I have
updated all INFO or higher level entries, but not yet DEBUG (😓)... So,
at a high level:
There is a slight change in log levels, effectively adding a new warning
level:
- DEBUG is still debug (ideally not for users but developers, though
this is something we need to work on)
- INFO is still info, though I've added more data here, effectively
making Syncthing more verbose by default (more on this below)
- WARNING is a new log level that is different from the _old_ WARNING
(more below)
- ERROR is what was WARNING before -- problems that must be dealt with,
and also bubbled as a popup in the GUI.
A new feature is that the logging level can be set per package to
something other than just debug or info, and hence I feel that we can
add a bit more things into INFO while moving some (in fact, most)
current INFO level warnings into WARNING. For example, I think it's
justified to get a log of synced files in INFO and sync failures in
WARNING. These are things that have historically been tricky to debug
properly, and having more information by default will be useful to many,
while still making it possible get close to told level of inscrutability
by setting the log level to WARNING. I'd like to get to a stage where
DEBUG is never necessary to just figure out what's going on, as opposed
to trying to narrow down a likely bug.
Code wise:
- Our logging object, generally known as `l` in each package, is now a
new adapter object that provides the old API on top of the newer one.
(This should go away once all old log entries are migrated.) This is
only for `l.Debugln` and `l.Debugf`.
- There is a new level tracker that keeps the log level for each
package.
- There is a nested setup of handlers, since the structure mandated by
`log/slog` is slightly convoluted (imho). We do this because we need to
do formatting at a "medium" level internally so we can buffer log lines
in text format but with separate timestamp and log level for the API/GUI
to consume.
- The `debug` API call becomes a `loglevels` API call, which can set the
log level to `DEBUG`, `INFO`, `WARNING` or `ERROR` per package. The GUI
is updated to handle this.
- Our custom `sync` package provided some debugging of mutexes quite
strongly integrated into the old logging framework, only turned on when
`STTRACE` was set to certain values at startup, etc. It's been a long
time since this has been useful; I removed it.
- The `STTRACE` env var remains and can be used the same way as before,
while additionally permitting specific log levels to be specified,
`STTRACE=model:WARN,scanner:DEBUG`.
- There is a new command line option `--log-level=INFO` to set the
default log level.
- The command line options `--log-flags` and `--verbose` go away, but
are currently retained as hidden & ignored options since we set them by
default in some of our startup examples and Syncthing would otherwise
fail to start.
Sample format messages:
```
2009-02-13 23:31:30 INF A basic info line (attr1="val with spaces" attr2=2 attr3="val\"quote" a=a log.pkg=slogutil)
2009-02-13 23:31:30 INF An info line with grouped values (attr1=val1 foo.attr2=2 foo.bar.attr3=3 a=a log.pkg=slogutil)
2009-02-13 23:31:30 INF An info line with grouped values via logger (foo.attr1=val1 foo.attr2=2 a=a log.pkg=slogutil)
2009-02-13 23:31:30 INF An info line with nested grouped values via logger (bar.foo.attr1=val1 bar.foo.attr2=2 a=a log.pkg=slogutil)
2009-02-13 23:31:30 WRN A warning entry (a=a log.pkg=slogutil)
2009-02-13 23:31:30 ERR An error (a=a log.pkg=slogutil)
```
---------
Co-authored-by: Ross Smith II <ross@smithii.com>
At a high level, this is what I've done and why:
- I'm moving the protobuf generation for the `protocol`, `discovery` and
`db` packages to the modern alternatives, and using `buf` to generate
because it's nice and simple.
- After trying various approaches on how to integrate the new types with
the existing code, I opted for splitting off our own data model types
from the on-the-wire generated types. This means we can have a
`FileInfo` type with nicer ergonomics and lots of methods, while the
protobuf generated type stays clean and close to the wire protocol. It
does mean copying between the two when required, which certainly adds a
small amount of inefficiency. If we want to walk this back in the future
and use the raw generated type throughout, that's possible, this however
makes the refactor smaller (!) as it doesn't change everything about the
type for everyone at the same time.
- I have simply removed in cold blood a significant number of old
database migrations. These depended on previous generations of generated
messages of various kinds and were annoying to support in the new
fashion. The oldest supported database version now is the one from
Syncthing 1.9.0 from Sep 7, 2020.
- I changed config structs to be regular manually defined structs.
For the sake of discussion, some things I tried that turned out not to
work...
### Embedding / wrapping
Embedding the protobuf generated structs in our existing types as a data
container and keeping our methods and stuff:
```
package protocol
type FileInfo struct {
*generated.FileInfo
}
```
This generates a lot of problems because the internal shape of the
generated struct is quite different (different names, different types,
more pointers), because initializing it doesn't work like you'd expect
(i.e., you end up with an embedded nil pointer and a panic), and because
the types of child types don't get wrapped. That is, even if we also
have a similar wrapper around a `Vector`, that's not the type you get
when accessing `someFileInfo.Version`, you get the `*generated.Vector`
that doesn't have methods, etc.
### Aliasing
```
package protocol
type FileInfo = generated.FileInfo
```
Doesn't help because you can't attach methods to it, plus all the above.
### Generating the types into the target package like we do now and
attaching methods
This fails because of the different shape of the generated type (as in
the embedding case above) plus the generated struct already has a bunch
of methods that we can't necessarily override properly (like `String()`
and a bunch of getters).
### Methods to functions
I considered just moving all the methods we attach to functions in a
specific package, so that for example
```
package protocol
func (f FileInfo) Equal(other FileInfo) bool
```
would become
```
package fileinfos
func Equal(a, b *generated.FileInfo) bool
```
and this would mostly work, but becomes quite verbose and cumbersome,
and somewhat limits discoverability (you can't see what methods are
available on the type in auto completions, etc). In the end I did this
in some cases, like in the database layer where a lot of things like
`func (fv *FileVersion) IsEmpty() bool` becomes `func fvIsEmpty(fv
*generated.FileVersion)` because they were anyway just internal methods.
Fixes#8247
This adds the ability to have multiple concurrent connections to a single device. This is primarily useful when the network has multiple physical links for aggregated bandwidth. A single connection will never see a higher rate than a single link can give, but multiple connections are load-balanced over multiple links.
It is also incidentally useful for older multi-core CPUs, where bandwidth could be limited by the TLS performance of a single CPU core -- using multiple connections achieves concurrency in the required crypto calculations...
Co-authored-by: Simon Frei <freisim93@gmail.com>
Co-authored-by: tomasz1986 <twilczynski@naver.com>
Co-authored-by: bt90 <btom1990@googlemail.com>
This makes the various protocol priorities configurable among the other
options. With this, it's possible to prefer QUIC over TCP for WAN
connections, for example. Both sides need to be similarly configured for
this to work properly.
The default priority order remains the same as previously (TCP, QUIC,
Relay, with LAN better than WAN).
To make this happen I made each dialer & listener more priority aware,
and moved the check for whether a connection is LAN or not into the
dialer / listener -- this is the new "lanChecker" type that's passed
around.
* lib/connections: Cache isLAN decision for later external access.
The check whether a remote device's address is on a local network
currently happens when handling the Hello message, to configure the
limiters. Save the result to the ConnectionInfo and pass it out as
part of the model's ConnectionInfo struct in ConnectionStats().
* gui: Use provided connection attribute to distinguish LAN / WAN.
Replace the dumb IP address check which didn't catch common cases and
actually could contradict what the backend decided. That could have
been confusing if the GUI says WAN, but the limiter is not actually
applied because the backend thinks it's a LAN.
Add strings for QUIC and relay connections to also differentiate
between LAN and WAN.
* gui: Redefine reception level icons for all connection types.
Move the mapping to the JS code, as it is much easier to handle
multiple switch cases by fall-through there.
QUIC is regarded no less than TCP anymore. LAN and WAN make the
difference between levels 4 / 3 and 2 / 1:
{TCP,QUIC} LAN --> {TCP,QUIC} WAN --> Relay LAN --> Relay WAN -->
Disconnected.
This truncates times meant for API consumption to second precision,
where fractions won't typically matter or add any value. Exception to
this is timestamps on logs and events, and of course I'm not touching
things like file metadata.
I'm not 100% certain this is an exhaustive change, but it's the things I
found by grepping and following the breadcrumbs from lib/api...
I also considered general-but-ugly solutions, like having the API
serializer itself do reflection magic or even regexps on returned
objects, but decided against it because aurgh...
This adds two new configuration options:
// The number of connections at which we stop trying to connect to more
// devices, zero meaning no limit. Does not affect incoming connections.
ConnectionLimitEnough int
// The maximum number of connections which we will allow in total, zero
// meaning no limit. Affects incoming connections and prevents
// attempting outgoing connections.
ConnectionLimitMax int
These can be used to limit the number of concurrent connections in
various ways.
This adds a statistic to track the last connection duration per device.
It isn't used for much in this PR, but it's available for #7223 to use
in deciding how to order device connection attempts (deprioritizing
devices that just dropped our connection the last time).
* lib/api, lib/connections, gui: Show connection error for disconnected devices (fixes#3345)
This adds functionality in the connetions service to track the last
error per address. That is in turn exposed in the /rest/system/status
API method, as that is also where we already show the listener status
from the connection service.
The GUI uses this info where it lists addresses, showing errors (if any)
in red underneath each address.
I also slightly refactored the existing status method on the connection
service to have a better name and return typed information.
* ok
* review
* formatting
* review
* lib/tlsutil: Enable TLS 1.3 when available, on test builds (fixes#5065)
This enables TLS 1.3 negotiation on Go 1.12 by setting the GODEBUG
variable. For now, this just gets enabled on test versions (those with a
dash in the version number).
Users wishing to enable this on production builds can set GODEBUG
manually.
The string representation of connections now includes the TLS version
and cipher suite. This becomes part of the log output on connections.
That is, when talking to an old client:
Established secure connection .../TLS1.2-TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256
and now potentially:
Established secure connection .../TLS1.3-TLS_AES_128_GCM_SHA256
(The cipher suite was there previously in the log output, but not the
TLS version.)
I also added this info as a new Crypto() method on the connection, and
propagate this out to the API and GUI, where it can be seen in the
connection address hover (although with bad word wrapping sometimes).
* wip
* wip
This avoids waiting until next ping and timeout until the connection is actually
closed both by notifying the peer of the disconnect and by immediately closing
the local end of the connection after that. As a nice side effect, info level
logging about dropped connections now have the actual reason in it, not a generic
timeout error which looks like a real problem with the connection.
This should address issue as described in https://forum.syncthing.net/t/stun-nig-party-with-paused-devices/10942/13
Essentially the model and the connection service goes out of sync in terms of thinking if we are connected or not.
Resort to model as being the ultimate source of truth.
I can't immediately pin down how this happens, yet some ideas.
ConfigSaved happens in separate routine, so it's possbile that we have some sort of device removed yet connection comes in parallel kind of thing.
However, in this case the connection exists in the model, and does not exist in the connection service and the only way for the connection to be removed
in the connection service is device removal from the config.
Given the subject, this might also be related to the device being paused.
Also, adds more info to the logs
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/4533
Well Tested(TM)
Introduces a potential issue where we always pick some connectable but dodgy connection that breaks
soon after the TLS handshake.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/4489
So there were some issues here. The main problem was that
model.Close(deviceID) was overloaded to mean "the connection was closed
by the protocol layer" and "i want to close this connection". That meant
it could get called twice - once *to* close the connection and then once
more when the connection *was* closed.
After this refactor there is instead a Closed(conn) method that is the
callback. I didn't need to change the parameter in the end, but I think
it's clearer what it means when it takes the connection that was closed
instead of a device ID. To close a connection, the new close(deviceID)
method is used instead, which only closes the underlying connection and
leaves the cleanup to the Closed() callback.
I also changed how we do connection switching. Instead of the connection
service calling close and then adding the connection, it just adds the
new connection. The model knows that it already has a connection and
makes sure to close and clean out that one before adding the new
connection.
To make sure to sequence this properly I added a new map of channels
that get created on connection add and closed by Closed(), so that
AddConnection() can do the close and wait for the cleanup to happen
before proceeding.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3490
This changes the BEP protocol to use protocol buffer serialization
instead of XDR, and therefore also the database format. The local
discovery protocol is also updated to be protocol buffer format.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3276
LGTM: AudriusButkevicius
This is in preparation for future changes, but also improves the
handling when talking to pre-v0.13 clients. It breaks out the Hello
message and magic from the rest of the protocol implementation, with the
intention that this small part of the protocol will survive future
changes.
To enable this, and future testing, the new ExchangeHello function takes
an interface that can be implemented by future Hello versions and
returns a version indendent result type. It correctly detects pre-v0.13
protocols and returns a "too old" error message which gets logged to the
user at warning level:
[I6KAH] 09:21:36 WARNING: Connecting to [...]:
the remote device speaks an older version of the protocol (v0.12) not
compatible with this version
Conversely, something entirely unknown will generate:
[I6KAH] 09:40:27 WARNING: Connecting to [...]:
the remote device speaks an unknown (newer?) version of the protocol
The intention is that in future iterations the Hello exchange will
succeed on at least one side and ExchangeHello will return the actual
data from the Hello together with ErrTooOld and an even more precise
message can be generated.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3289
Just an optimization. Required exposing the priority from the factory,
so made that an interface with an extra method instead of just a func
type.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3071
1. Removes separate relay lists and relay clients/services, just makes it a listen address
2. Easier plugging-in of other transports
3. Allows "hot" disabling and enabling NAT services
4. Allows "hot" listen address changes
5. Changes listen address list with a preferable "default" value just like for discovery
6. Debounces global discovery announcements as external addresses change (which it might alot upon starting)
7. Stops this whole "pick other peers relay by latency". This information is no longer available,
but I don't think it matters as most of the time other peer only has one relay.
8. Rename ListenAddress to ListenAddresses, as well as in javascript land.
9. Stop serializing deprecated values to JSON
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/2982
