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

MYST-274 Session duration #123

Merged
merged 3 commits into from Jan 30, 2018

Conversation

Projects
None yet
2 participants
@donce
Copy link
Contributor

commented Jan 29, 2018

No description provided.

@donce donce force-pushed the feature/MYST-274-session-duration branch from cd1aa0f to 0649f88 Jan 29, 2018

@donce donce force-pushed the feature/MYST-274-session-duration branch from 0649f88 to 3e09cd8 Jan 30, 2018

duration, err := ce.statsKeeper.GetSessionDuration()
if err != nil {
newErr := fmt.Errorf("unable to retrieve session duration: %v", err.Error())
utils.SendError(writer, newErr, http.StatusUnprocessableEntity)

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 30, 2018

Member

Unprocessable entity error is meant to use for http request body validation and is not allowed for GET requests, not to report some internal state problems. Maybe duration can be simply 0 if session is not started? Do we really need to handle that "special" case?

This comment has been minimized.

Copy link
@donce

donce Jan 30, 2018

Author Contributor

Simplified to return zero in this case - no custom http status left.

http.StatusOK, `{
"bytesSent": 0,
"bytesReceived": 0,
"durationSeconds": 60

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 30, 2018

Member

what about just "sessionDuration"? It can be just convention - sent,received are reported as bytes, duration is reported as seconds. Api consumer when can reformat to any precision needed.

This comment has been minimized.

Copy link
@donce

donce Jan 30, 2018

Author Contributor

bytesSent and bytesReceived specifies that it's bytes in the name, while duration does not. At first I implemented it in duration myself, but having raw number without explanation about value seemed confusing - also durationSeconds is more consistent with bytesSent/bytesReceived.

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 30, 2018

Member

In that case: maybe sentBytes, receivedBytes and sessionSeconds? :) or vice versa. More consistent


// GetSessionDuration returns elapsed time from marked session start
func (keeper *sessionStatsKeeper) GetSessionDuration() (time.Duration, error) {
if keeper.sessionStart == nil {

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 30, 2018

Member

Do we really need to throw errors here and force any caller to handle them? It's just for statistics. Maybe we can simply return zero in case session is not started.

This comment has been minimized.

Copy link
@donce

donce Jan 30, 2018

Author Contributor

Simplified to return zero in this case.

@tadovas
Copy link
Member

left a comment

LGTM

@donce donce merged commit cc476d7 into master Jan 30, 2018

@donce donce deleted the feature/MYST-274-session-duration branch Jan 30, 2018

@zolia zolia referenced this pull request Feb 7, 2019

Open

Feature/basic value transfer to provider with details #706

5 of 18 tasks complete
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.