Skip to content
This repository was archived by the owner on Feb 7, 2026. It is now read-only.

fix: logged errors are reported to error reporting#122

Merged
DominicKramer merged 18 commits intogoogleapis:masterfrom
DominicKramer:fix/report-logged-errors
Aug 27, 2018
Merged

fix: logged errors are reported to error reporting#122
DominicKramer merged 18 commits intogoogleapis:masterfrom
DominicKramer:fix/report-logged-errors

Conversation

@DominicKramer
Copy link
Copy Markdown
Contributor

@DominicKramer DominicKramer commented Aug 15, 2018

Fixes: #121

@DominicKramer DominicKramer requested a review from a team August 15, 2018 21:24
@ghost ghost assigned DominicKramer Aug 15, 2018
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 15, 2018
kjin
kjin reviewed Aug 15, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@kjin kjin left a comment

Choose a reason for hiding this comment

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

Same comments as in the logging-winston PR.

@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 15, 2018 •
edited
Loading

Codecov Report

Merging #122 into master will increase coverage by 0.42%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #122      +/-   ##
==========================================
+ Coverage   84.83%   85.26%   +0.42%     
==========================================
  Files           3        3              
  Lines         343      353      +10     
  Branches       21       22       +1     
==========================================
+ Hits          291      301      +10     
  Misses         52       52
Impacted Files Coverage Δ
test/index.ts 95.78% <100%> (+0.14%) ⬆️
src/index.ts 94.66% <100%> (+0.14%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7132e8b...3ec6e59. Read the comment docs.

kjin
kjin approved these changes Aug 15, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@kjin kjin left a comment

Choose a reason for hiding this comment

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

LGTM when CI passes.

@DominicKramer DominicKramer force-pushed the fix/report-logged-errors branch from 8191f7d to d5da5f2 Compare August 21, 2018 00:45
@DominicKramer DominicKramer mentioned this pull request Aug 21, 2018
Merged
@DominicKramer DominicKramer force-pushed the fix/report-logged-errors branch from 1b44c90 to 5d3b4cd Compare August 21, 2018 22:48
@DominicKramer DominicKramer force-pushed the fix/report-logged-errors branch from 5d3b4cd to 5472080 Compare August 22, 2018 00:44
ofrobots
ofrobots reviewed Aug 22, 2018
View reviewed changes
Comment thread src/index.ts Outdated
// or higher will not be displayed in the Error Reporting
// console
if (!this.serviceContext.service) {
this.serviceContext.service = 'default';

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

ofrobots
ofrobots previously requested changes Aug 22, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@ofrobots ofrobots left a comment

Choose a reason for hiding this comment

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

Making the request for changes explicit. Neither default nor unknown are not a good service names to fall back to. Users would not necessarily be able to find where their errors go.

@ofrobots
Copy link
Copy Markdown
Contributor

ofrobots commented Aug 23, 2018

We talked in person about the next steps

  1. Throw on bad config provided by users
  2. Implement auto-detection of service context when running on Google Cloud.

I think the former should be addressed in this PR, and the latter be implemented as a follow-on feature.

@ofrobots ofrobots mentioned this pull request Aug 23, 2018
Merged
@ghost ghost assigned JustinBeckwith Aug 23, 2018
@DominicKramer DominicKramer mentioned this pull request Aug 23, 2018
Closed
@DominicKramer
Copy link
Copy Markdown
Contributor Author

DominicKramer commented Aug 23, 2018

@ofrobots I've updated this PR with the features we discussed. I've opened issue #135 for the follow-on work. PTAL

@ghost ghost assigned ofrobots Aug 23, 2018
@DominicKramer
Copy link
Copy Markdown
Contributor Author

DominicKramer commented Aug 27, 2018

@ofrobots and I discussed this PR in person and, after updating this PR to not modify the default section of the README, this PR is ready to land.

@JustinBeckwith JustinBeckwith dismissed ofrobots’s stale review August 27, 2018 17:33

Discussed in person

@DominicKramer DominicKramer merged commit c35aa22 into googleapis:master Aug 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@kjin kjin kjin approved these changes

@ofrobots ofrobots ofrobots left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@ofrobots ofrobots

@JustinBeckwith JustinBeckwith

@DominicKramer DominicKramer

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@DominicKramer @ofrobots @kjin @JustinBeckwith @googlebot