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

Programmatically Environment override doesn't work #43

Open
simonespa opened this issue Jun 9, 2020 · 4 comments
Open

Programmatically Environment override doesn't work #43

simonespa opened this issue Jun 9, 2020 · 4 comments
Labels
wontfix This will not be worked on

Comments

@simonespa
Copy link

Summary

According to #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
Copy link
Member

jaredcnance commented Jun 9, 2020

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.

@jaredcnance jaredcnance added the bug Something isn't working label Jun 9, 2020
@simonespa
Copy link
Author

@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
Copy link
Member

jaredcnance commented Jun 10, 2020

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

@simonespa
Copy link
Author

Thanks @jaredcnance.

@jaredcnance jaredcnance added wontfix This will not be worked on and removed bug Something isn't working labels Jul 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants