-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
better user-agent handling #702
Conversation
if version == "(devel)" { | ||
ClientVersion = bi.Main.Path | ||
} else { | ||
ClientVersion = fmt.Sprintf("%s@%s", bi.Main.Path, bi.Main.Version) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the path/version of the main module (application), not libp2p itself.
63a4a4c
to
009eeba
Compare
Instead of using a global variable. This also: * Adds an option to the identify service to set the user agent. * Removes the ability to pass an identify service to NewHost as any reasonable Identify service already needs to be constructed with an instance of the host.
This should help us improve network stats on who's using libp2p.
009eeba
to
e337633
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
// ClientVersion is the default user agent. | ||
// | ||
// Deprecated: Set this with the UserAgent option. | ||
var ClientVersion = "github.com/libp2p/go-libp2p" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's just call this go-libp2p
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure but users using go mod will still use their full paths by default. E.g., github.com/libp2p/go-libp2p-examples@v0.1.1
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh ok, nevermind then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will increase the size of the identify messages by 15 bytes. It's ok but worth noting.
} | ||
version := bi.Main.Version | ||
if version == "(devel)" { | ||
ClientVersion = bi.Main.Path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's keep the devel
designator somewhere in the reported agent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anything built manually (i.e., built from a manually checked-out source tree) will have a (devel)
version. It doesn't mean it's actually a development version.
To get a proper version, one would have to build with go get github.com/libp2p/go-libp2p-examples@latest
(i.e., build with go mod).
Users that care can override this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we suffix with @unknown?
@raulk this:
|
// ClientVersion is the default user agent. | ||
// | ||
// Deprecated: Set this with the UserAgent option. | ||
var ClientVersion = "github.com/libp2p/go-libp2p" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will increase the size of the identify messages by 15 bytes. It's ok but worth noting.
} | ||
version := bi.Main.Version | ||
if version == "(devel)" { | ||
ClientVersion = bi.Main.Path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we suffix with @unknown?
@@ -47,7 +67,8 @@ const transientTTL = 10 * time.Second | |||
// * Our IPFS Agent Version | |||
// * Our public Listen Addresses | |||
type IDService struct { | |||
Host host.Host | |||
Host host.Host | |||
UserAgent string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping this exported could lead to races if the user modifies the value at runtime. Not a biggie, but just pointing it out.
@@ -31,9 +33,27 @@ const ID = "/ipfs/id/1.0.0" | |||
|
|||
// LibP2PVersion holds the current protocol version for a client running this code | |||
// TODO(jbenet): fix the versioning mess. | |||
// XXX: Don't change this till 2020. You'll break all go-ipfs versions prior to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should make this configurable too. Many projects using libp2p don't care about compatibility with IPFS and their networks are deliberately segregated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #714
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, modulo:
- some missing godocs -- fixed in a subsequent commit.
- suffix unknown versions with
@unknown
, to normalise the format of that field. - Make LibP2PVersion configurable and dedup with UserAgent #714 (collateral to this).
Thanks for waiting for my sign off, @Stebalien!
This PR improves user-agent handling by: