-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add Error Reporting Client #1946
Conversation
.. doctest:: | ||
|
||
>>> from gcloud import error_reporting | ||
>>> from |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
@waprin Should this be targeting the |
69603fb
to
1f59346
Compare
@tseaver comments addressed, travis passes. I am totally fine to put this in |
@waprin Would you prefer to open a separate PR targeting the |
I will recreate the PR if that is in fact the branch you want it in. |
Hmm, looking at it again, I'm not sure that branch is right: your changes here are pretty independent of anything directly related to logging, especially from the user's point-of-view. Will there eventually be separate API endpoints for error reporting? |
Hmm, looks like there are endpoints, but they don't get mapped onto this client, and they are (confusingly) not related to reporting that an error happened, but rather on generating reports of errors. Do you see your client adding support for those endpoints? |
There will be an endpoint to report an error eventually, at that point we may want to change the code here to talk directly to it rather than through logging (for quota management, etc) Support for the other endopints is a nice-to-have but not a priority. Biggest reason to add some now is just so we can write system tests for the existing code. But yes I envision this is where that code will go. If you think in the interest of completeness or testing we add it now then I can work on it. |
For the other endpoints, we are talking to the Veneer team and they can probably be generated. What really matters is an idiomatic library and method to report errors. |
executable, job, or Google App Engine module name. This | ||
field is expected to have a low number of values that are | ||
relatively stable over time, as opposed to version, | ||
which can be changed whenever new code is deployed. |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
513e35e
to
58ef6a5
Compare
vkit will generate a gRPC client but sounds like we won't be using it in the near future until other grpc complexities are resolved. @tseaver As mentioned in comments I agree we need a client that actually takes all the available params, and the helper functions are built on top of that. I was taking baby steps to get reviews, your thoughts on where all this code should go etc but if you'd prefer I do it all in this one PR that's fine, or we can do a separate feature branch, or something else. Also while the logging handler and this are mostly orthogonal, there is a slight overlap in that we want a handler option that reports the context of logging.error messages to Error Reporting. |
>>> client = error_reporting.Client(project='my-project', credentials=creds) | ||
|
||
Error Reporting associates errors with a service, which is an identifier for an executable, | ||
App Engine module, or job. The default service is "python", but a default can be specified |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
'serviceContext': { | ||
'service': service, | ||
}, | ||
'message': '{0} : {1}'.format(message, traceback.format_exc()) |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
286c39a
to
10024e3
Compare
@tseaver @daspecster understand you guys are busy but are you planning on getting this and #1942 merged? I have more PRs on the way but nervous that I haven't been able to get anything into master yet. |
@dhermes was sitting next to me a few minutes ago and had no opinion, but sure waiting for Tres makes sense. There's no big rush either just checking yall plan to merge this stuff etc |
LGTM |
cool ty, hold on let me remove accidental shebangs |
@waprin I think I broke the build somehow. It's failing on importing intersphinx. I'll check it out. |
Perhaps it wasn't me, but it's strange either way. |
@daspecster can merge unless you think lint stuff blocks it |
I'll wait to hear back from @dhermes. |
Don't block this on lint stuff |
Ok I'm going to merge it then. |
This is another baby step, this time towards addressing #1881.
Once again, probably goes into a feature branch.
First of all, this just piggybacks on the Logging client by directly extending it. Error Reporting has its own API, but is still rolling out support for the most important call, which is to actually post an error.
This PR just adds a single function which reports the exception detail through Error Reporting using the Logging API.
If we want to add a system test, then we should add the functionality to actually get the details about the reported error.
https://cloud.google.com/error-reporting/reference/rest/v1beta1/projects.events/list
If we think that's necessary I'll do it.
There is some intersection between Error Reporting and Logging in that we might want a logging handler to report to Error Reporting on a logging call of severity error and we want to enable error reporting to use logging's async transports. But I'll wait till both of these branches get fleshed out more before working on that.
/cc @steren