Add logger builder

[Bingjiling]

Issue #, if available:

Description of changes:

• Add log builder in interface package

• Deployed and tested in an AWS account

• Example log output

Unhandled error:

One string:
this.logger.info('tada-info');

One string and one object:
this.logger.error('tada-error', {name: 'tada-error', desc: 'this is a error in JSON with multiple item' });

Two string and two objects:

this.logger.warn(
                        'two string and two json',
                        'string 2',
                        {
                            name: 'tada-warning',
                            desc: 'this is a warning in JSON',
                        },
                        {
                            name: 'tada-warning2',
                            desc: 'this is a warning2 in JSON',
                        },
                    );

Log error and stack:

const e = new Error('somethings up, log the error and stack trace');
this.logger.error(e, e.stack);

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

[AllanHodkinson]

Hi,
I've been looking over this PR with interest as we've already implemented a similar logging mechanism for FWoA ourselves.

Would a better approach be to define the logger in the fhirConfig interface as you've done, but put the implementation into a different component e.g. the FWoA deployment, or a new FWoA logger component?
Thanks,
Allan

[Bingjiling]

Hi,
I've been looking over this PR with interest as we've already implemented a similar logging mechanism for FWoA ourselves.

Would a better approach be to define the logger in the fhirConfig interface as you've done, but put the implementation into a different component e.g. the FWoA deployment, or a new FWoA logger component?
Thanks,
Allan

Hi Allan,

Thanks for your interest in FWoA! It's a great point not having implementation in the interface package. We talked about
avoiding it during design review as well. However, the logging component itself is very small for now so it's a bit excessive to create a separate component for it. If the logging component does become more complicated or there're other shared functionalities needed, a separate component will be considered.

Meanwhile, I'm curious what approach did you take in creating a logging mechanism?

Thanks,
Yanyu

[AllanHodkinson]

Hi,
I've been looking over this PR with interest as we've already implemented a similar logging mechanism for FWoA ourselves.
Would a better approach be to define the logger in the fhirConfig interface as you've done, but put the implementation into a different component e.g. the FWoA deployment, or a new FWoA logger component?
Thanks,
Allan

Hi Allan,

Thanks for your interest in FWoA! It's a great point not having implementation in the interface package. We talked about
avoiding it during design review as well. However, the logging component itself is very small for now so it's a bit excessive to create a separate component for it. If the logging component does become more complicated or there're other shared functionalities needed, a separate component will be considered.

Meanwhile, I'm curious what approach did you take in creating a logging mechanism?

Thanks,
Yanyu

Hi Yanyu,
Thanks for getting back in touch.

We have a slightly different approach here at Black Pear; we're using the FWoA interface and router, but for the search & persistence components we've built an architecture that allows us to plug in different modules dependent on what services the customer needs. So we could provide a general FHIR server by plugging in the existing FWoA Dynamo & ES persistence components. Or we can plug in one of our own search & persistence components, for example to interface with a 3rd party clinical system that doesn't use FHIR and provide a FHIR facade for it.

We've added several components including logging, audit & cache into FWoA by wrapping the components in a wrapper a bit like this:

export class QFSearchService implements Search {
    private helper: HelperComponent;

    constructor(helper: HelperComponent) {
        this.helper = helper;
    }

    async typeSearch(request: TypeSearchRequest): Promise<SearchResponse> {
        helper.logger.log('Hello from Black Pear!');
        helper.audit.recordEvent(...);
        let data = helper.cache.get('keyForCachedData');        
    }
.....
}

then we build a our deployment with something along the lines of:

const logger:Logger = new Logger();
const myHelper:HelperComponent = new Helper(logger);
const searchComponent:Search = new QFSearchService(myHelper);
let config: FhirConfig = {
        configVersion: 1.0,
        ....
        profile: {
                   systemSearch: searchComponent:Search, 
       }        
}
generateServerlessRouter(fhirConfig...);
...

Our logging mechanism is pretty simple, at the moment we are just logging to cloudwatch with the standard levels. But with our approach we could switch in Winston, or could add a custom component e.g SNS publish, as required.
Thanks,
Allan

[Bingjiling]

Hi @AllanHodkinson,

Thanks for explaining your approach in detail! It does make sense to wrap functionalities like log, cache and audit into a helper component and shared it in its entirety in the architecture you described. A follow up question on the helper component, is it a standalone repo or is it part of another existing repo in your architecture?

Also, as part of this change, I was planning to remove the log levels export from the interface package, and remove the log level setting from the deployment package. I'm wondering if removing the settings or any of the changes in this PR would be a breaking change for you? I want to make sure we don't make a negative impact here.

Thanks,
Yanyu

[AllanHodkinson]

Hi @AllanHodkinson,

Thanks for explaining your approach in detail! It does make sense to wrap functionalities like log, cache and audit into a helper component and shared it in its entirety in the architecture you described. A follow up question on the helper component, is it a standalone repo or is it part of another existing repo in your architecture?

Also, as part of this change, I was planning to remove the log levels export from the interface package, and remove the log level setting from the deployment package. I'm wondering if removing the settings or any of the changes in this PR would be a breaking change for you? I want to make sure we don't make a negative impact here.

Thanks,
Yanyu

Hi Yanyu,
Our helper components are part of a bigger sdk that includes most of our deployment setup. It's structured in a way to allow us to pass in any search/persistence/auth component (which follows the FWoA interface) on sdk initialization and build an appropriate Lambda zip file.

Thanks for the heads up on the logging levels change, we're not currently using this. We have an env var that allows us to set this directly.
Thanks,
Allan

[rsmayda]

Before we merge this in can we tag and release the existing changes so that they are not bundled with this breaking change?

Existing changes are tagged and released. Dismiss this review as Robert is on vacay so let's leave him alone :)