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

feat: sdk runtime metrics #545

Merged
merged 15 commits into from Feb 13, 2020
Merged

feat: sdk runtime metrics #545

merged 15 commits into from Feb 13, 2020

Conversation

@hacdias
Copy link
Member

hacdias commented Feb 13, 2020

This closes #372 and it is divided into two main commits:

  1. f9b05aa: adds the actual code for collecting runtime metrics using Prometheus.
    1. runenv.MustExportPrometheus which returns an address to the entrypoint where the metrics can be collected. This function panics in case of error and starts on a random free port.
    2. runenv.HTTPPeriodicSnapshots which periodically checks for new metrics. It accepts an address, a duration and the output file. The output file might include a $TIME placeholder which will be replaced by a unix timestamp. If not, the timestamp will be appended to the filename.
  2. 0eab061: adds a small example.
    1. ./testground run single placebo/metrics -i 1 -b exec:go -r local:exec
    2. ./testground collect -r local:exec RUN_ID

Questions:

  1. Since I'm adding this to runenv, couldn't I just store the address on RunEnv and avoid passing it to HTTPPeriodicSnapshots?
hacdias added 2 commits Feb 13, 2020
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias hacdias requested review from nonsense, Stebalien and raulk Feb 13, 2020
@hacdias

This comment has been minimized.

Copy link
Member Author

hacdias commented Feb 13, 2020

I'm hitting the too many unstructured assets; current: 32 error. Is there a way to stop keeping track of them?

Edit: what if I go with You can also manually create output assets/directories under re.TestOutputsPath. and the user just defines a path for the metrics and we save everything there timestamp.out?

sdk/runtime/metrics.go Outdated Show resolved Hide resolved
sdk/runtime/metrics.go Outdated Show resolved Hide resolved
hacdias added 3 commits Feb 13, 2020
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias

This comment has been minimized.

Copy link
Member Author

hacdias commented Feb 13, 2020

After talking to @nonsense, we decided to make a few changes:

  • MustExportPrometheus now returns a listener that the caller can close and that can be used to get the address.
  • HTTPPeriodicSnapshots takes a context that can be used to stop the goroutine that makes the periodic calls.

I also decided to manually create the directory for the metrics instead of using CreateRawAsset to overcome the 32 limit. It also gives more flexibility.

sdk/runtime/metrics.go Outdated Show resolved Hide resolved
Copy link
Member

nonsense left a comment

LGTM

hacdias added 2 commits Feb 13, 2020
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
plans/placebo/main.go Outdated Show resolved Hide resolved
plans/placebo/main.go Outdated Show resolved Hide resolved
@@ -1,12 +1,77 @@
github.com/alecthomas/template v0.0.0-20160405071501-a0175ee3bccc/go.mod h1:LOuyumcjzFXgccqObfd/Ljyb9UuFJ6TxHnclSeseNhc=

This comment has been minimized.

Copy link
@raulk

raulk Feb 13, 2020

Member

Ooooof, that's a lot of transitive dependencies. I thought prometheus would be lighter. Just to make sure we're not leaking anything unnecessary here, could you run go mod tidy once more?

This comment has been minimized.

Copy link
@hacdias

hacdias Feb 13, 2020

Author Member

I ran it about 3 times already.

sdk/runtime/metrics.go Outdated Show resolved Hide resolved
sdk/runtime/metrics.go Outdated Show resolved Hide resolved
sdk/runtime/metrics.go Outdated Show resolved Hide resolved
sdk/runtime/metrics.go Show resolved Hide resolved
sdk/runtime/metrics.go Show resolved Hide resolved
sdk/runtime/metrics.go Outdated Show resolved Hide resolved
hacdias and others added 4 commits Feb 13, 2020
Co-Authored-By: Raúl Kripalani <raul@protocol.ai>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias hacdias requested a review from raulk Feb 13, 2020
hacdias added 4 commits Feb 13, 2020
@hacdias

This comment has been minimized.

Copy link
Member Author

hacdias commented Feb 13, 2020

I'm merging this since all the feedback has been addressed in the meanwhile and everything is passing now.

@hacdias hacdias merged commit 184ad52 into master Feb 13, 2020
2 of 4 checks passed
2 of 4 checks passed
codecov/patch 0% of diff hit (target 11.41%)
Details
codecov/project 11.17% (-0.25%) compared to a1a6d48
Details
Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details
@hacdias hacdias deleted the sdk/runtime-metrics branch Feb 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

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