awslabs / aws-embedded-metrics-node

Amazon CloudWatch Embedded Metric Format Client Library
https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/CloudWatch_Embedded_Metric_Format.html
Apache License 2.0
253 stars 35 forks source link

Programmatically Environment override doesn't work #43

Open simonespa opened 4 years ago

simonespa commented 4 years ago

Summary

According to https://github.com/awslabs/aws-embedded-metrics-node/pull/19 you can now override the environment detection by either using

const { Configuration } = require("aws-embedded-metrics");
Configuration.environmentOverride = "Local";

or

export AWS_EMF_ENVIRONMENT=Local

When I export the environment variable it works and I can see the agent logging to stdout, when I try the first approach, I still get the following error:

(node:783) UnhandledPromiseRejectionWarning: Error: connect ECONNREFUSED 0.0.0.0:25888
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1141:16)
(node:783) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 13)
(node:783) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

Details

I've got the following helper module

import { metricScope, Unit, Configuration } from 'aws-embedded-metrics';
import config from 'config';

const environment = config.get('environment');
const namespace = config.get('namespace');

Configuration.logGroupName = namespace;
Configuration.environmentOverride = 'Local';

export const pageRequestLogger = metrics => {
  return async pageRequestEvent => {
    const {
      requestCount,
      errorCount,
      responseTime,
      pageType,
      event,
      processPid,
      request,
      response,
      log,
      logTrace
    } = pageRequestEvent;
    metrics.setNamespace(namespace);
    metrics.putMetric('RequestCount', requestCount, Unit.Count);
    metrics.putMetric('ErrorCount', errorCount, Unit.Count);
    metrics.putMetric('ResponseTime', responseTime, Unit.Milliseconds);
    metrics.setDimensions({ PageType: pageType });

    metrics.setProperty('event', event);
    metrics.setProperty('processPid', processPid);
    metrics.setProperty('request', request);
    metrics.setProperty('response', response);
    metrics.setProperty('log', log);
    metrics.setProperty('logTrace', logTrace);
  };
};

export const logPageRequest = metricScope(pageRequestLogger);

Environment

The code runs on a Centos7 Docker container Node: 12.18.0 NPM: 6.14.4

jaredcnance commented 4 years ago

We have this test which was intended to demonstrate this, however I believe the issue is actually caused by environment detection kicking off at the time the module is loaded and then getting cached and never re-evaluated. Unfortunately, due to module hoisting the fix isn't as simple as changing the configuration prior to importing the metricScope.

simonespa commented 4 years ago

@jaredcnance thanks for looking into it. I'm happy that I can override at least with the env variable.

Would it be useful to temporarily flag this in the docs by either annotating that it currently doesn't work or temporarily removing the programmatic approach from it? Just so people are aware that in the meantime they have to use the ENV variable, unless the fix is quicker than a doc change of course 😄

jaredcnance commented 4 years ago

Yes, that’s a good point. We can remove this from the documentation for now. Updated in #44.

simonespa commented 4 years ago

Thanks @jaredcnance.