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

fix(analytics): correctly extract statusCode from failed request #5618

merged 6 commits into from Apr 29, 2020


Copy link

@iartemiev iartemiev commented Apr 29, 2020

Issue #, if available:
Fixes #5423

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@iartemiev iartemiev self-assigned this Apr 29, 2020
Copy link

@sammartinez sammartinez left a comment

LGTM 🌮, the implementation is good, I believe we need to adjust a couple of the unit tests. So I will keep it approved but should look into why the unit tests are failing

Copy link

@codecov codecov bot commented Apr 29, 2020

Codecov Report

Merging #5618 into master will decrease coverage by 0.00%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5618      +/-   ##
- Coverage   72.92%   72.91%   -0.01%     
  Files         197      197              
  Lines       11532    11532              
  Branches     2252     2176      -76     
- Hits         8410     8409       -1     
- Misses       2951     2972      +21     
+ Partials      171      151      -20     
Impacted Files Coverage Δ
packages/analytics/src/Providers/EventBuffer.ts 30.55% <0.00%> (ø)
...ges/analytics/src/Providers/AWSPinpointProvider.ts 75.53% <100.00%> (-0.31%) ⬇️
packages/auth/src/OAuth/OAuth.ts 48.12% <0.00%> (ø)
packages/core/src/Credentials.ts 31.48% <0.00%> (ø)
packages/analytics/src/Analytics.ts 66.86% <0.00%> (ø)
packages/datastore/src/sync/outbox.ts 25.00% <0.00%> (ø)
packages/datastore/src/storage/storage.ts 67.59% <0.00%> (ø)
packages/core/src/OAuthHelper/GoogleOAuth.ts 32.65% <0.00%> (ø)
packages/core/src/Util/Reachability.native.ts 37.50% <0.00%> (ø)
... and 8 more

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 f5db38d...d278d0e. Read the comment docs.

Copy link

@sammartinez sammartinez left a comment

Reapproving, LGTM 🌮

@iartemiev iartemiev merged commit e11e938 into aws-amplify:master Apr 29, 2020
2 of 3 checks passed
@iartemiev iartemiev deleted the issue5423 branch Apr 29, 2020
logger.debug('updateEndpoint failed', err);
const statusCode = err.$metadata && err.$metadata.httpStatusCode;

logger.error('updateEndpoint failed', err);
Copy link

@alexandersandberg alexandersandberg May 8, 2020

Choose a reason for hiding this comment

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

The endpoints seem to update successfully now, but this message is still in the log and is now throwing an error despite the success:

error message

Copy link
Contributor Author

@iartemiev iartemiev May 8, 2020

Choose a reason for hiding this comment

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

You're right, that's a regression introduced in this PR. Will restore it to logger.debug

Choose a reason for hiding this comment

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

There is a similar problem with the 'Missing address' warning now turned into an error, but I noticed that your PR reverts that as well.


Copy link

@github-actions github-actions bot commented Jun 11, 2021

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels or Discussions for those types of questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
None yet
None yet

Successfully merging this pull request may close these issues.

3 participants