Skip to content

fix(metrics): emit warning when default dimensions are overwritten #4222

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

Merged

Conversation

uttam282005
Copy link
Contributor

Summary

Adds warning messages to the Metrics setDefaultDimensions method when overwriting existing default dimensions, ensuring consistency and parity with addDimension/addDimensions methods.

Changes

This pull request improves the robustness of the setDefaultDimensions method in the Metrics utility by:

  • Moved setEnvConfig() and setConsole() calls from setOptions() to the constructor to ensure logger is initialized before any method uses it.
  • Ensuring it gracefully handles undefined, null, and invalid values for default dimensions.
  • Updated the setDefaultDimensions() method in the Metrics utility to emit a warning when a default dimension is being overwritten.
  • Logging a warning if an invalid dimension name or value is provided (i.e., non-string or empty).
  • Adding new unit tests to verify the above behaviors, including:
    • Ensuring that calling setDefaultDimensions(undefined) results in immediate return, with no warning emitted and only default dimensions applied.
    • Verifying that invalid dimension values (undefined, null, empty strings) or names are skipped, a warning is emitted in the console, and only valid dimensions are set.

This PR addresses #4134 by improving input validation and error reporting for the setDefaultDimensions method in the Metrics module, to prevent invalid dimension names/values from being set.

Issue number: #4134

Tests Added

The following tests were added:

it('returns immediately if dimensions is undefined', () => {
  // Prepare
  const metrics = new Metrics({
    singleMetric: true,
    namespace: DEFAULT_NAMESPACE,
  });
  // Act
  metrics.setDefaultDimensions(undefined);
  metrics.addMetric('myMetric', MetricUnit.Count, 1);
  // Assess: No warning, only default dimensions/service
  expect(console.warn).not.toHaveBeenCalled();
  expect(console.log).toHaveEmittedEMFWith(
    expect.objectContaining({
      service: 'hello-world',
    })
  );
});

it.each([
  { value: undefined, name: 'valid-name' },
  { value: null, name: 'valid-name' },
  { value: '', name: 'valid-name' },
  { value: 'valid-value', name: '' },
])(
  'skips invalid default dimension values in setDefaultDimensions ($name)',
  ({ value, name }) => {
    // Arrange
    const metrics = new Metrics({
      singleMetric: true,
      namespace: DEFAULT_NAMESPACE,
    });

    // Act
    metrics.setDefaultDimensions({
      validDimension: 'valid',
      [name as string]: value as string,
    });

    metrics.addMetric('test', MetricUnit.Count, 1);
    metrics.publishStoredMetrics();

    // Assert
    expect(console.warn).toHaveBeenCalledWith(
      `The dimension ${name} doesn't meet the requirements and won't be added. Ensure the dimension name and value are non empty strings`
    );

    expect(console.log).toHaveEmittedEMFWith(
      expect.objectContaining({ validDimension: 'valid' })
    );

    expect(console.log).toHaveEmittedEMFWith(
      expect.not.objectContaining({ [name]: value })
    );
  }
);

setDefaultDimensions Update

  • Modified to emit a warning if a dimension name or value does not meet requirements (non-empty string).

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

@pull-request-size pull-request-size bot added the size/L PRs between 100-499 LOC label Jul 28, 2025
@boring-cyborg boring-cyborg bot added metrics This item relates to the Metrics Utility tests PRs that add or change tests labels Jul 28, 2025
Copy link
Contributor

@sdangol sdangol left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I've added a few minor comments

…Record check

Adds a unit test to ensure setDefaultDimensions logs a warning and
returns early
when passed a non-object value (e.g., a string), validating the runtime
isRecord guard.
Copy link
Contributor

@sdangol sdangol left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the contribution and for making the changes @uttam282005. Really appreciate it.

Copy link

@svozza svozza merged commit a933a6a into aws-powertools:main Jul 29, 2025
46 checks passed
@svozza
Copy link
Contributor

svozza commented Jul 29, 2025

Great work @uttam282005! This will be included in today's release.

@sdangol
Copy link
Contributor

sdangol commented Jul 29, 2025

Hi @uttam282005, we were trying to do a release today but our e2e tests were failing. We realized that there were duplicate logs being generated. This seems to be because of the setDefaultDimensions method being called in multiple places without invoking it explicitly. I've created an issue for this: #4227
Would you like to work on a fix for this?

@uttam282005
Copy link
Contributor Author

Thanks for flagging this @sdangol, and apologies for the oversight. I’d be happy to work on a fix.

@svozza
Copy link
Contributor

svozza commented Jul 29, 2025

Absolutely no need to apologise! This is why we have E2E tests because it's very hard to know how all the pieces in a module interact when you're working on a specific piece of the code base.

uttam282005 added a commit to uttam282005/powertools-lambda-typescript that referenced this pull request Jul 30, 2025
uttam282005 added a commit to uttam282005/powertools-lambda-typescript that referenced this pull request Aug 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
metrics This item relates to the Metrics Utility size/L PRs between 100-499 LOC tests PRs that add or change tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Maintenance: Add warning for overwriting dimensions in setDefaultDimensions method
3 participants