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

Acceptance tests: Collect and save metrics #2247

Merged
merged 4 commits into from
Dec 14, 2018

Conversation

worxli
Copy link
Contributor

@worxli worxli commented Dec 13, 2018

Also contributes to #2153.

This change is Reviewable

@worxli worxli changed the title Acceptance tests: Collect and log metrics Acceptance tests: Collect and save metrics Dec 13, 2018
@worxli worxli force-pushed the collect-metrics branch 2 times, most recently from be9ebb7 to 2394cb3 Compare December 13, 2018 16:57
Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

Reviewed 19 of 19 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @worxli)


acceptance/common.sh, line 11 at r1 (raw file):

}

collect_metrics() {

I think we should start to document lib function. At least describe the arguments.
Also this seems to only work for BR and SIGs.


acceptance/common.sh, line 17 at r1 (raw file):

(jq '.BorderRouters | to_entries[] | .key' $1)

Possibly simpler: jq -r '.BorderRouters | keys | .[]' $1 note the -r removes the quotes so you no longer need the remove_quotes function


python/topology/docker.py, line 176 at r1 (raw file):

                sciond = get_default_sciond_path(ISD_AS(topo["ISD_AS"]))
                entry['command'].append('--spki_cache_dir=cache')
                entry['command'].append('--prom=[0.0.0.0]:%s' % CS_PROM_PORT)

We need 0.0.0.0 because of docker here?

Copy link
Contributor Author

@worxli worxli left a comment

Choose a reason for hiding this comment

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

Reviewable status: 15 of 19 files reviewed, 3 unresolved discussions (waiting on @lukedirtwalker)


acceptance/common.sh, line 11 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

I think we should start to document lib function. At least describe the arguments.
Also this seems to only work for BR and SIGs.

Done.


acceptance/common.sh, line 17 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…
(jq '.BorderRouters | to_entries[] | .key' $1)

Possibly simpler: jq -r '.BorderRouters | keys | .[]' $1 note the -r removes the quotes so you no longer need the remove_quotes function

Done.


python/topology/docker.py, line 176 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

We need 0.0.0.0 because of docker here?

Yes. We bind to 0.0.0.0 so we can access the metrics on the container IP from the host.

Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 4 of 4 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@worxli worxli merged commit 34b4c8e into scionproto:master Dec 14, 2018
@worxli worxli deleted the collect-metrics branch December 14, 2018 13:13
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.

2 participants