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

Basic workers stats #148

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

Conversation

karitra
Copy link

@karitra karitra commented Jan 16, 2018

For preliminary review.

Gopher gurus, @3Hren, @antmat, PTAL.

Metrics (for porto and procfs):

  • cpu load: percents
  • memory: bytes count
  • network: tx, rx bytes count (for porto only, but yet system wide stat provided)

Setup (via config file)

{
...
  "isolate": { 
      "porto" {
            "type": "porto",
            "args": {
                "workersmetrics": {
                    "period": "5s"
                }
            }
        },
        "process": {
            "type": "process",
            "args": {
                "workersmetrics": {
                    "period": "5s"
                }
            }
        },
       ...
  }
}

Zero poll period (default) switches metrics poll off.

TODO:

  • per worker network stat (seems not trivial to implement)
  • docker isolation stats (not in priority, separate PR)
  • tests

@karitra karitra changed the title Basci workers stats Basic workers stats Jan 16, 2018
return nil, err
}

return d.onWorkersMetrics(uuids)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are u sure that not
return d.onWorkersMetrics(uuids), nil
?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I bet he's sure)

Copy link
Collaborator

@antmat antmat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather big PR - still not fully reviewed.


sz, err = r.ReadArrayHeader()
if err != nil {
return uuids, err
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just return?

return nil, err
}

return d.onWorkersMetrics(uuids)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I bet he's sure)


startTime := time.Now()

sendMetricsFunc := func(metrics MetricsResponse) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe better to make as a standalone function or method?

)

if d == nil {
log.G(d.ctx).Error("strange: dispatch is `nil`")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can it happen? If no - maybe remove, so we can get a crash when logic is being somehow violated?

select {
case <- ctx.Done():
return
case <-time.After(interval):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe it' better to use time.Ticker it should spawn less goroutines

@karitra karitra force-pushed the per_worker_stat branch 2 times, most recently from 4a493a3 to e19d3a7 Compare January 19, 2018 14:31
@codecov-io
Copy link

codecov-io commented Jan 25, 2018

Codecov Report

Merging #148 into master will decrease coverage by 7.04%.
The diff coverage is 0.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #148      +/-   ##
==========================================
- Coverage   35.98%   28.93%   -7.05%     
==========================================
  Files          41       45       +4     
  Lines        3185     3981     +796     
==========================================
+ Hits         1146     1152       +6     
- Misses       1758     2548     +790     
  Partials      281      281
Impacted Files Coverage Δ
isolate/isolate.go 14.28% <ø> (ø) ⬆️
isolate/initialdispatch.go 36.63% <0%> (-12.06%) ⬇️
isolate/container_metrics.go 0% <0%> (ø)
isolate/conn_handler.go 0% <0%> (ø) ⬆️
isolate/process/metrics_gatherer.go 0% <0%> (ø)
isolate/docker/box.go 71.82% <0%> (-0.81%) ⬇️
isolate/container_metrics_gen.go 0% <0%> (ø)
isolate/porto/box.go 0% <0%> (ø) ⬆️
isolate/porto/metrics_gatherer.go 0% <0%> (ø)
isolate/process/box.go 59.37% <5%> (-10.76%) ⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2371b3e...e27c3b4. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants