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

feat(logging): CW client, deviceId & NetworkMonitor #12907

Merged

Conversation

Samaritan1011001
Copy link
Member

@Samaritan1011001 Samaritan1011001 commented Jan 26, 2024

Description of changes

This PR brings in the following three main pieces:

  1. [logging] CloudWatch client -- contains the log, flushLogs and configure methods
    a. Example format of the log message sent to CloudWatch -- [${logLevel}] ${namespace}/${category}: ${message}
  2. [core] deviceId impl
  3. [core] NetworkMonitoring impl -- module to run a handler when the device comes back online.

There are a couple of TODOs in the CW client that is left for further addition to the implementation.

Issue #, if available

Description of how you validated changes

  1. React app using the CW provider to log
  2. Checked local store to see saved logs
  3. Unit tests confirm behavior of proposed changes

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)

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

@Samaritan1011001 Samaritan1011001 changed the title Feat/cw client feat(logging): CW client, deviceId & NetworkMonitor Jan 26, 2024
@Samaritan1011001 Samaritan1011001 marked this pull request as ready for review January 30, 2024 01:54
@Samaritan1011001 Samaritan1011001 requested review from a team as code owners January 30, 2024 01:54
Copy link
Member

@ashwinkumar6 ashwinkumar6 left a comment

Choose a reason for hiding this comment

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

Great work @Samaritan1011001 !!
Just a couple of questions and nits for refactoring based on the previous PR merged

Copy link
Member

@ashika112 ashika112 left a comment

Choose a reason for hiding this comment

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

I am good overall with this. I think we need to re-arrange and organize some utils function but good to followup with another PR.

Copy link
Member

@ashwinkumar6 ashwinkumar6 left a comment

Choose a reason for hiding this comment

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

Thanks @Samaritan1011001, Looking good
Just a couple of nits and questions

I'm good with also adding TODOs and getting this merged

packages/logging/src/providers/cloudwatch/types/types.ts Outdated Show resolved Hide resolved
packages/logging/src/providers/cloudwatch/types/types.ts Outdated Show resolved Hide resolved
},
};

export const cloudWatchProvider: CloudWatchProvider = {
Copy link
Member

Choose a reason for hiding this comment

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

nit
I'm wondering if we should break up this entire object into separate functions.

For internal logs the API that exposes the CW object could just be something like

export const cloudWatchProvider: LoggingProvider = {
    log: (logParams: LogParams) => {
        logToCloudWatch(logParams);
    }
};

Also with the recent design proposal, we're planning to just expose CW provider enable/disable instead of the entire category level enable/disable so I think we should be fine

ashwinkumar6 pushed a commit to ashwinkumar6/amplify-js that referenced this pull request Feb 8, 2024
@Samaritan1011001 Samaritan1011001 merged commit 0234167 into aws-amplify:central-logger Feb 9, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants