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

Added fetchQuality to the proposals endpoint with a fake data #444

Merged
merged 9 commits into from Oct 18, 2018

Conversation

Projects
None yet
4 participants
@soffokl
Copy link
Member

commented Oct 14, 2018

No description provided.

@soffokl soffokl self-assigned this Oct 14, 2018

@soffokl soffokl requested review from Waldz, zolia and vkuznecovas Oct 14, 2018

@soffokl soffokl requested a review from tadovas as a code owner Oct 14, 2018

for i := range rp.Proposals {
rp.Proposals[i].Quality = &serviceQuality{
Connects: connectsHistory{
Success: 100,

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 14, 2018

Member
  1. No point to mock for all proposals, mock really working proposals only
  2. Not point to mock in Desktop artefact, because redeploying real data to desktop machine will be hard. We need to mock in URL, so that we can affect desktops easily later with real data
@@ -65,6 +65,19 @@ type proposalRes struct {

// qualitative service definition
ServiceDefinition serviceDefinitionRes `json:"serviceDefinition"`

// Quality of the service
Quality *serviceQuality `json:"quality,omitempty"`

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 14, 2018

Member

Sometime null sometimes structure {}?

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 15, 2018

Author Member
  1. If the quality exists it returns it as a struct;
  2. If it does not exist - no quality filed present in the JSON.

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 16, 2018

Member

Requirement is: If fetchQuality is given then full structure quality: {} should always be present.
Is it so now?

2 different cases are visible here: #429

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 17, 2018

Author Member

Changed it, now if fetchQuality flag is given, it prints full struct even with zero values.

@soffokl soffokl force-pushed the proposals-quality branch from 9931f97 to bc5ca1f Oct 16, 2018

@soffokl

This comment has been minimized.

Copy link
Member Author

commented Oct 16, 2018

@Waldz @vkuznecovas @tadovas @zolia this PR is ready for review.

@@ -34,6 +35,7 @@ var TestnetDefinition = NetworkDefinition{
"https://testnet-api.mysterium.network/v1",
"testnet-broker.mysterium.network",
"https://ropsten.infura.io",
"https://morqa.mysterium.network/api/v1",

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 16, 2018

Member

Should be deployed with same convention https://testnet-morqa.mysterium.network/api/v1

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 17, 2018

Author Member

Changed.

@@ -153,6 +155,22 @@ func (mApi *mysteriumAPI) FindProposals(providerID string) ([]dto_discovery.Serv
return proposalsResponse.Proposals, nil
}

// ProposalsQuality returns a list of proposals connection quality
func (mApi *mysteriumAPI) ProposalsQuality() ([]dto.QualityConnects, error) {

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 16, 2018

Member

Dont couple to DiscoveryClient, should be separate http client

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 17, 2018

Author Member

Decoupled.

@@ -65,6 +65,19 @@ type proposalRes struct {

// qualitative service definition
ServiceDefinition serviceDefinitionRes `json:"serviceDefinition"`

// Quality of the service
Quality *serviceQuality `json:"quality,omitempty"`

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 16, 2018

Member

Requirement is: If fetchQuality is given then full structure quality: {} should always be present.
Is it so now?

2 different cases are visible here: #429


// QualityConnects represents a single proposal connection quality description
type QualityConnects struct {
Proposal QualityProposal `json:"proposal"`

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 16, 2018

Member

Why service definition is here. It should not. MORQA service should have just proposalID primary key to identity proposal.

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 16, 2018

Member

Proposal ID (current):
providerID: 0x2

Proposal ID (implementing now):

proposalID: {
  providerID: 0x2,
  serviceType: "openvpn"
}

Proposal ID (future):

proposalID: {
  providerID: 0x2,
  serviceType: "openvpn",
  serialNumber: 2
}

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 17, 2018

Author Member

Removed definition of these structs.

// QualityConnects represents a single proposal connection quality description
type QualityConnects struct {
Proposal QualityProposal `json:"proposal"`
CountAll int `json:"countAll"`

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 16, 2018

Member

I dont think we need to map MORQA response to local DTOs, because new field adding will be complex.
Lets just map MORQA response by proposalID and proxy it in Tequilapi

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 17, 2018

Author Member

Now it matches MORQA responses and just proxy it.


addQualityToRes := func(p proposalRes) proposalRes { return p }
if fetchQuality == "true" {
addQualityToRes, err = addQuality(pe.mysteriumMorqaClient)

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 17, 2018

Member

Quality oracle should be absolutely optional feature.
It means, if it's offline, proposal listing is still possible.

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 18, 2018

Author Member

It now only logs errors.

@@ -35,3 +37,8 @@ type Client interface {

SendSessionStats(sessionId session.ID, sessionStats dto.SessionStats, signer identity.Signer) (err error)
}

// MorqaClient allows to interact with a quality oracle service (MORQA)
type MorqaClient interface {

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 17, 2018

Member

Interface should be on caller side, not implementation

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 18, 2018

Author Member

Interface now defined in the metrics package.

* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

package server

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 17, 2018

Member

Lets start having package quality and quality/oraqle_morqa

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 18, 2018

Author Member

Moved to the metrics and metrics/oracle packages

}
return proposalsResArry
}

type proposalsEndpoint struct {
mysteriumClient server.Client
mysteriumClient server.Client
mysteriumMorqaClient server.MorqaClient

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 17, 2018

Member

It should be QualityOracle interface, and user can You our Morqa service, but can use another quality provider also

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 18, 2018

Author Member

Renamed interface.

}
},
"quality": {
"proposal": {

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 17, 2018

Member

Dont neet to repeat proposalID structure here, it's in upper structure

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 18, 2018

Author Member

Done.

@@ -0,0 +1,25 @@
/*
* Copyright (C) 2017 The "MysteriumNetwork/node" Authors.

This comment has been minimized.

Copy link
@zolia

zolia Oct 17, 2018

Member

Prob should be 2018

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 18, 2018

Author Member

Fixed.


// ServiceQualityResponse represents response from the quality oracle service
type ServiceQualityResponse struct {
Connects []json.RawMessage `json:"connects"`

This comment has been minimized.

Copy link
@zolia

zolia Oct 17, 2018

Member

please consider using naming suggested by me at #429

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 18, 2018

Author Member

Renamed to metrics.


proposalsQuality := make(map[string]json.RawMessage, len(connects))
for _, c := range connects {
var info struct{ Proposal struct{ ProviderID string } }

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 17, 2018

Member

Lets have proposalID structure with 2 fields from beginning, be consistent with:

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 18, 2018

Author Member

Done.

@soffokl soffokl requested review from Waldz and zolia Oct 18, 2018

}

out, err := json.Marshal(metrics)
json, err := metrics.Parse(c, &proposal)

This comment has been minimized.

Copy link
@zolia

zolia Oct 18, 2018

Member

we should rename connects to metrics and m variable.

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 18, 2018

Author Member

Renamed to receivedMetrics since metrics conflicts with the imported package name.

@zolia
Copy link
Member

left a comment

LGTM

soffokl added some commits Oct 16, 2018

@soffokl soffokl dismissed stale reviews from Waldz and zolia via 7c1b88c Oct 18, 2018

@soffokl soffokl force-pushed the proposals-quality branch from cd5419f to 7c1b88c Oct 18, 2018

@Waldz

Waldz approved these changes Oct 18, 2018

@zolia

zolia approved these changes Oct 18, 2018

@Waldz Waldz merged commit 993b5fc into master Oct 18, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@Waldz Waldz deleted the proposals-quality branch Oct 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.