Added ability to configure the url where the errors get sent#29
Added ability to configure the url where the errors get sent#29steren merged 1 commit intoGoogleCloudPlatform:masterfrom
Conversation
trevtrich
commented
Dec 8, 2017
trevtrich
commented
- This is primarily necessary to support web clients that would like to avoid using api keys.
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
|
|
||
| **Q: Are private source maps supported?** A: No, see [#4](https://github.com/GoogleCloudPlatform/stackdriver-errors-js/issues/4) | ||
|
|
||
| **Q: Can I use this without an API key?** A: Yes, see below |
There was a problem hiding this comment.
Can you add that the custom URL must implement the report API endpoint: https://cloud.google.com/error-reporting/reference/rest/v1beta1/projects.events/report
|
|
||
| **Q: Are private source maps supported?** A: No, see [#4](https://github.com/GoogleCloudPlatform/stackdriver-errors-js/issues/4) | ||
|
|
||
| **Q: Can I use this without an API key?** A: Yes, see below |
There was a problem hiding this comment.
I would prefer to create a dedicated README section rather than a Q.
|
CLAs look good, thanks! |
|
All great suggestions. I've updated accordingly. One thing to bear in mind is that when I built the new tests I ran into an issue with the urls having to include the text 'clouderrorreporting'. Thus, you'll see that my new test, though a different overall url, does contain that text. It seems that there is some state lingering on all the tests due to the timeout in the beforeeach. If you drop an console printout into the filter and printout the urls, you'll see that they build up as more tests are run. I didn't want to completely rip apart the tests as part of my first commit so I went the route I did, but thought I'd pass along and potentially do this as a followup when I have time. |
|
|
||
| ## Configuring without an API key | ||
|
|
||
| If you are in a situation where an API key is not an option, you can configure the endpoint that errors are sent to with the following: |
There was a problem hiding this comment.
Instead of "you can configure the endpoint that errors are sent to". You should also explain what's the idea:
"you can forward these errors to an endpoint on your server, and report them server-side. You can can configure the endpoint that errors are sent to...."
| version: '<my-service-version>' // (optional) | ||
| }); | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Please detail more what's expected to be put in targetUrl: is it compatible with relative URLs or should I provide an absolute one?
| }); | ||
|
|
||
| describe('Reporting errors', function () { | ||
| beforeEach(function() { |
There was a problem hiding this comment.
Instead of deleting this beforeEach. Can you put your new test into a new describe? For example describe('Reporting errors with custom URL
There was a problem hiding this comment.
I wasn't sure what pattern you guys usually conformed to. Guess I chose the wrong one! Ha, is this what you were looking for or should I separate the new on out into it's own entirely?
- This is primarily necessary to support web clients that would like to avoid using api keys.
|
I believe I've addressed all comments. Let me know if there are some further changes you'd like to see. |
|
Thanks! |
|
I'll wait for #30 and do a new release |
3 participants
