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

Extract GCP Monitoring #192

Merged
merged 20 commits into from
Feb 12, 2019
Merged

Extract GCP Monitoring #192

merged 20 commits into from
Feb 12, 2019

Conversation

marwan-at-work
Copy link
Contributor

@marwan-at-work marwan-at-work commented Feb 9, 2019

  • Created a new gcputil package (following ioutil/serverutil pattern)
  • Moved helpful initialization functions out of server/kit into the new package so that SimpleServer or really any Go program can initialize metrics/tracing.
  • Added a new function RegisterOpenCensus which makes it easier for the user to just call this function, without having to manually get the metadata, insert them into the opts, get the opts, insert the opts into the Exporter.
  • I bet server/kit can probably use RegisterOpenCensus with some refactoring in the future

fixes #185

@coveralls
Copy link

coveralls commented Feb 9, 2019

Coverage Status

Coverage increased (+0.6%) to 46.98% when pulling ba9794e on marwan-at-work:gcp into 9eaa369 on NYTimes:master.

Copy link
Contributor

@jprobinson jprobinson left a comment

Choose a reason for hiding this comment

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

Made some comments regarding the package name and how things get registered. Lemme know what you think.

gcputil/gcputil.go Outdated Show resolved Hide resolved
gcputil/gcputil.go Outdated Show resolved Hide resolved
gcputil/gcputil.go Outdated Show resolved Hide resolved
gcputil/gcputil.go Outdated Show resolved Hide resolved
@marwan-at-work
Copy link
Contributor Author

@jprobinson all comments should be addressed, happy to revise more

Copy link
Contributor

@jprobinson jprobinson left a comment

Choose a reason for hiding this comment

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

I've made a few notes but this is a great start!

observe/observe.go Outdated Show resolved Hide resolved
observe/observe.go Outdated Show resolved Hide resolved
observe/observe.go Outdated Show resolved Hide resolved
observe/observe.go Outdated Show resolved Hide resolved
server/kit/kitserver.go Outdated Show resolved Hide resolved
observe/observe.go Outdated Show resolved Hide resolved
observe/observe.go Outdated Show resolved Hide resolved
@marwan-at-work
Copy link
Contributor Author

@jprobinson all comments addressed

server/kit/kitserver.go Outdated Show resolved Hide resolved
server/kit/log.go Outdated Show resolved Hide resolved
@jprobinson
Copy link
Contributor

Mind also adding a mention of this new package in the repo README?

observe/observe.go Outdated Show resolved Hide resolved
@marwan-at-work
Copy link
Contributor Author

@jprobinson i fixed the Go doc, there was already a mention of those variables in the main README, but i also added them to the server/kit README

@marwan-at-work
Copy link
Contributor Author

@jprobinson fixed the onErr message, let me know if there's anything else that needs updating. Thanks!

observe/observe.go Outdated Show resolved Hide resolved
observe/observe.go Outdated Show resolved Hide resolved
@jprobinson
Copy link
Contributor

Ah shoot, sorry. I just noticed we're referencing the mention in the root README.md of the project. The blurb used for the project level doc should be fine.

Copy link
Contributor

@jprobinson jprobinson left a comment

Choose a reason for hiding this comment

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

📈 ! 🌯 ! Thanks for your patience, @marwan-at-work!

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.

Extract App Engine From server/kit
5 participants