search

GoogleCloudPlatform / stackdriver-errors-js

Client-side JavaScript exception reporting library for Cloud Error Reporting
https://cloud.google.com/error-reporting/
Apache License 2.0
362 stars 54 forks source link

[pr] Use Custom HttpRequestContext from Context #75

Closed jackywangKaggle closed 4 years ago

jackywangKaggle commented 4 years ago

Use HttpRequestContext that is passed in from Context during StackdriverErrorReporter.start method in https://github.com/GoogleCloudPlatform/stackdriver-errors-js#setup-for-javascript

steren commented 4 years ago

Thanks for the PR.

What are the use cases for overloading the URL and user agent?

Can you update the README with these new parameters and when to use them?

jackywangKaggle commented 4 years ago

Thanks for the PR.

What are the use cases for overloading the URL and user agent?

Can you update the README with these new parameters and when to use them?

Thanks @steren, the URL and the user agent were originally set by the report method in https://github.com/GoogleCloudPlatform/stackdriver-errors-js/pull/75/files#diff-7511f8b6167fd3b0d9d201f3728e97f1L112 (which I now moved to the start method). I think it makes sense as a default value suggested in this PR and allow users to customize them if desired. For example, I know we didn't set those values explicitly, but it became helpful to the what type of user agent we are looking at.

Will update README

steren commented 4 years ago

I am still struggling to see the use case. This library is intended at client-side error reporting from web apps / web pages. This is not a library to send arbitrary data to the Error Reporting API. Why would a developer want to customize the URL or the user agent? Please explain more.

Also, this should not be done in the .start() method, as I expect the values to change over time.

Overall, I think I am against this change. You should have opened an issue to discuss the proposed feature. As it is today, I will not be merging the PR.

jackywangKaggle commented 4 years ago

I am still struggling to see the use case. This library is intended at client-side error reporting from web apps / web pages. This is not a library to send arbitrary data to the Error Reporting API. Why would a developer want to customize the URL or the user agent? Please explain more.

Also, this should not be done in the .start() method, as I expect the values to change over time.

Overall, I think I am against this change. You should have opened an issue to discuss the proposed feature. As it is today, I will not be merging the PR.

Thanks @steren, I see your point regarding not allowing users to override the UserAgent, Url and along the same line prob not a good idea to allow setting Method. The reason why I'm trying to make this change is so we can capture the RemoteIp information in our client-side error reporting, since that can help us understand where the error is coming from and take appropriate actions.

steren commented 4 years ago

I prefer not introducing these options to keep things simple. The use case does not seem strong enough.

As you can see, the code of this library is extremely simple.

I recommend you to write your own library that fits your needs, potentially forking this one.