fix(sdk-metrics): Add missing catch and handle error in promise of PeriodicExportingMetricReader

[jj22ee]

Which problem is this PR solving?

PeriodicExportingMetricReader's doExport method will throw an Error if metrics export failed, and is expected to be caught within this try-catch.

This method is called in two locations within the same if-else, but is only caught/handled in the else while not being caught/handled in the if. Although the if-else is within a try-catch, one of the doExport invocations is called within a Promise.then(), which isn't being caught right now

• Location 1 (doExport's error is NOT caught):
  

  opentelemetry-js/packages/sdk-metrics/src/export/PeriodicExportingMetricReader.ts

  Lines 141 to 145 in 6515ed8

  	resourceMetrics.resource
  	.waitForAsyncAttributes?.()
  	.then(doExport, err =>
  	diag.debug('Error while resolving async portion of resource: ', err)
  	);

• Location 2 (doExport's error is caught):
  

  opentelemetry-js/packages/sdk-metrics/src/export/PeriodicExportingMetricReader.ts

  Line 147 in 6515ed8

  await doExport();

This PR ensures that doExport in Location 1 is caught.

Short description of the changes

• Add catch to the promise that runs doExport in the then statement

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Test in my application involving PeriodicExportingMetricReader with small metric export interval and many resource detectors (which causes resourceMetrics.resource.asyncAttributesPending to be true)

export OTEL_METRIC_EXPORT_INTERVAL=50     // 50ms export interval makes it easier to trigger this error, but is unrealistic
export OTEL_NODE_RESOURCE_DETECTORS='process,env,host'

• Before fix, I trigger crash

Accessing resource attributes before async attributes settled
/Users/-------/Documents/sample_project/node_modules/@opentelemetry/sdk-metrics/build/src/export/PeriodicExportingMetricReader.js:76
                throw new Error(`PeriodicExportingMetricReader: metrics export failed (error ${result.error})`);
                      ^

Error: PeriodicExportingMetricReader: metrics export failed (error Error: 14 UNAVAILABLE: No connection established. Last error: connect ECONNREFUSED 127.0.0.1:4318 (2024-09-20T22:43:31.324Z))
    at doExport (/Users/-------/Documents/sample_project/node_modules/@opentelemetry/sdk-metrics/build/src/export/PeriodicExportingMetricReader.js:76:23)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)
    at runNextTicks (node:internal/process/task_queues:64:3)
    at process.processImmediate (node:internal/timers:449:9)
    at process.callbackTrampoline (node:internal/async_hooks:130:17)

• After fix, error is logged

Accessing resource attributes before async attributes settled
{"stack":"Error: PeriodicExportingMetricReader: metrics export failed (error Error: 14 UNAVAILABLE: No connection established. Last error: connect ECONNREFUSED 127.0.0.1:4318 (2024-09-21T00:02:04.200Z))\n    at doExport (/Users/-------/Documents/sample_project/node_modules/@opentelemetry/sdk-metrics/build/src/export/PeriodicExportingMetricReader.js:76:23)\n    at processTicksAndRejections (node:internal/process/task_queues:95:5)\n    at runNextTicks (node:internal/process/task_queues:64:3)\n    at process.processImmediate (node:internal/timers:449:9)\n    at process.callbackTrampoline (node:internal/async_hooks:130:17)\n    at async PeriodicExportingMetricReader._doRun (/Users/-------/Documents/sample_project/node_modules/@opentelemetry/sdk-metrics/build/src/export/PeriodicExportingMetricReader.js:84:13)\n    at async PeriodicExportingMetricReader._runOnce (/Users/-------/Documents/sample_project/node_modules/@opentelemetry/sdk-metrics/build/src/export/PeriodicExportingMetricReader.js:55:13)","message":"PeriodicExportingMetricReader: metrics export failed (error Error: 14 UNAVAILABLE: No connection established. Last error: connect ECONNREFUSED 127.0.0.1:43158(2024-09-21T00:02:04.200Z))","name":"Error"}

Checklist:

• Followed the style guidelines of this project
• Unit tests have been added
• Documentation has been updated

[pichlermarc]

Ah, I see:

if the attributes are pending AND the promise form waitForAsyncAttributes() resolves, then doExport() is called, and IF that rejects (that happens when when the exporter returns failure) then the the promise is unhandled.

Good catch 👍

I feel like the whole code-block should've really been:

    // Avoid scheduling a promise to make the behavior more predictable and easier to test
    if (resourceMetrics.resource.asyncAttributesPending) {
      try {
        await resourceMetrics.resource.waitForAsyncAttributes?.();
      } catch (err) {
        diag.debug('Error while resolving async portion of resource: ', err);
        return;
      }
    }

    await doExport();

to ensure that the error reprorted to globalErrorHandler actually has the correct stacktrace on it.

Please also add a changelog (./CHANGELOG.md) entry. Thanks for fixing this. 🙂

[jj22ee]

ty! Added changelog.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.92%. Comparing base (f8ab559) to head (bd39689).
Report is 13 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5006      +/-   ##
==========================================
+ Coverage   93.39%   93.92%   +0.53%     
==========================================
  Files          46      310     +264     
  Lines         712     8138    +7426     
  Branches      120     1633    +1513     
==========================================
+ Hits          665     7644    +6979     
- Misses         47      494     +447     

Files with missing lines	Coverage Δ
...etrics/src/export/PeriodicExportingMetricReader.ts	92.98% <ø> (ø)

... and 265 files with indirect coverage changes