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

Go proc metrics collector #55

Open
wants to merge 7 commits into
base: apptuit-master
Choose a base branch
from

Conversation

talonx
Copy link

@talonx talonx commented May 23, 2018

Fix for #49

from __future__ import print_function
import traceback

import requests

Choose a reason for hiding this comment

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

Will this require a dependency on python-requests.deb/rpm?

Copy link
Author

Choose a reason for hiding this comment

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

As discussed, it's already part of the installer dependencies.

from collectors.lib import utils

COLLECTION_INTERVAL_SECONDS = 15
SKIP_MEM_KEYS = ['PauseNs', 'PauseEnd', 'BySize']

Choose a reason for hiding this comment

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

Can we move these to the yml?

Copy link
Author

Choose a reason for hiding this comment

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

The collector decides what metrics it will emit, this should not be user configurable.

@@ -0,0 +1,64 @@
#!/usr/bin/env python

from __future__ import print_function

Choose a reason for hiding this comment

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

How come other collectors did not have this?

Copy link
Author

Choose a reason for hiding this comment

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

Fixed



def main(argv):
expvar_url = goprocconf.get_expvar_url()

Choose a reason for hiding this comment

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

First line of main is usually utils.drop_privileges()

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, added

except:
utils.err("Unexpected error: %s" % sys.exc_info()[0])
traceback.print_exc()
exit(13)

Choose a reason for hiding this comment

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

Instead of exiting anytime we get a connection error, I suggest we change the workflow to:

  1. Before while(true): Try connecting to the endpoint if it errors out then exit - This is the scenario where the URL in the yml is not on this server
  2. In the while(true): On connection error, we log the error and continue the loop - This is the scenario where the URL is correct (we checked before the loop), but there is a transient error

Copy link
Author

Choose a reason for hiding this comment

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

Done

print("memstats.%s %s %s" % (mk, ts, mv))
elif k != 'cmdline':
_print_nested_metrics(k, v, ts)
sys.stdout.flush()

Choose a reason for hiding this comment

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

Convention has been to flush once at the end of each loop, please move it just before the sleep

Copy link
Author

Choose a reason for hiding this comment

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

Done

conf/goproc.yml Outdated
collector:
name: goproc
config:
expvar_url: http://localhost:3001/debug/vars

Choose a reason for hiding this comment

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

Please change the path to collector.config.endpoints.url so thatwe could support multiple URL scraping in future

Copy link
Author

Choose a reason for hiding this comment

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

Done. It's a list of dictionaries now in the YAML but we just parse the first element in the list.

import sys
import time

from collectors.etc import goprocconf

Choose a reason for hiding this comment

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

We are trying to move away from py based conf to yml based conf. Could you inline the yml reading in this collector?

Copy link
Author

Choose a reason for hiding this comment

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

Done

@rshivane
Copy link

Could you rename the collector to go_expvar.py?

pause_sum += pause_ns[index]
gc_count += 1
index = index - 1
_print_metric('memstats.PauseNs.sum', ts_seconds, pause_sum)

Choose a reason for hiding this comment

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

The name as per Metrics 2.0 convention would be memstats.Pause.total.nanos

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