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

Provide a way to give extra data to the tracker on announce #595

Merged
merged 4 commits into from Feb 29, 2016

Conversation

@yciabaud
Copy link
Contributor

yciabaud commented Feb 4, 2016

This PRs is dependent on webtorrent/bittorrent-tracker#107 and webtorrent/torrent-discovery#15 to provide a callback for the client to get updated params and send it along with the announce message to the tracker.

It sends updated download stats (uploaded/downloaded/letf and adds a callback in th options to extend it with additionnal data.

The callback is called before every announce to get updated data from the client.

@feross

This comment has been minimized.

Copy link
Member

feross commented Feb 8, 2016

What's your use case for sending extra parameters to the tracker? Some kind of authentication?

@@ -90,6 +90,7 @@ function Torrent (torrentId, opts) {
this.pieces = []
this._selections = []
this._critical = []
this._getExtraAnnounceOpts = opts.getExtraAnnounceOpts

This comment has been minimized.

Copy link
@feross

feross Feb 8, 2016

Member

I'd rather name this consistently with the bittorrent-tracker option. getAnnounceOpts

uploaded: self.uploaded,
downloaded: self.downloaded,
left: self.length - self.downloaded,
extraAnnounceOpts: self._getExtraAnnounceOpts && self._getExtraAnnounceOpts()

This comment has been minimized.

Copy link
@feross

feross Feb 8, 2016

Member

Instead of passing the user's custom announce opts separately in this extraAnnounceOpts property, can we merge their properties with the uploaded, downloaded, and left params?

Thinking something like this:

getAnnounceOpts: function () {
  return extend({
    uploaded: self.uploaded,
    downloaded: self.downloaded,
    left: self.length - self.downloaded
  }, self._getExtraAnnounceOpts && self._getExtraAnnounceOpts())
})

This ordering will mean that the user's values for uploaded, downloaded, and left (if they exist) will take precedence.

@feross

This comment has been minimized.

Copy link
Member

feross commented Feb 8, 2016

LGTM 👍 after the inline comments and webtorrent/bittorrent-tracker#107 comments are addressed.

@yciabaud

This comment has been minimized.

Copy link
Contributor Author

yciabaud commented Feb 8, 2016

For now I am sending per wire statistics in order to track data transfer between peers, the bitfield to compute video playing stats (see when the consumer seeks and stops the video...) but later I want my tracker to provide an optimized peerlist to reduce latency. So I will probably retrieve more metrics.

feross added a commit that referenced this pull request Feb 29, 2016
Provide a way to give extra data to the tracker on announce
@feross feross merged commit 76fe99f into webtorrent:master Feb 29, 2016
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@feross

This comment has been minimized.

Copy link
Member

feross commented Mar 1, 2016

Good news! This is released as 0.80.0.

feross added a commit that referenced this pull request Mar 1, 2016
@yciabaud yciabaud deleted the yciabaud:announce-extension branch Mar 1, 2016
@yciabaud

This comment has been minimized.

Copy link
Contributor Author

yciabaud commented Mar 1, 2016

Thank you for finishing this.

@feross

This comment has been minimized.

Copy link
Member

feross commented Mar 2, 2016

Of course! Thanks for the PR!

@lock

This comment has been minimized.

Copy link

lock bot commented May 4, 2018

This thread has been automatically locked because it has not had recent activity. To discuss futher, please open a new issue.

@lock lock bot locked as resolved and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.