Skip to content

Conversation

@david-luna
Copy link
Contributor

@david-luna david-luna commented Aug 7, 2024

Which problem is this PR solving?

As suggested by @blumamir this PR aggregates the previous PRs to add sync detectors in the @opentelemetry/resource-detector-aws package. Previous PRs will be closed

Short description of the changes

  • add detectors for Beanstalk, EC2, ECS, EKS & Lambda complying with the DetectorSync interface.
  • add tests
  • make use of named exports

import * as assert from 'assert';
import * as sinon from 'sinon';
import { awsBeanstalkDetector, AwsBeanstalkDetector } from '../../src';
import { awsBeanstalkDetector, AwsBeanstalkDetectorSync } from '../../src';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note for reviewer: awsBeanstalkDetector is delegating the job to awsBeanstalkDetectorSync so we need to apply the stubs into AwsBeanstalkDetectorSync class

.resolves(JSON.stringify(data));

const resource = await awsBeanstalkDetector.detect();
await resource.waitForAsyncAttributes?.();
Copy link
Contributor Author

@david-luna david-luna Aug 7, 2024

Choose a reason for hiding this comment

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

note for reviewer: now awsBeanstalkDetector.detect() resolves to a promise before the async attributes are resolved so we need to await. You'll see the same change in other tests

@codecov
Copy link

codecov bot commented Aug 7, 2024

Codecov Report

Attention: Patch coverage is 96.11650% with 12 lines in your changes missing coverage. Please review.

Project coverage is 90.51%. Comparing base (dfb2dff) to head (b7febdf).
Report is 207 commits behind head on main.

Files Patch % Lines
...e-detector-aws/src/detectors/AwsEksDetectorSync.ts 91.46% 7 Missing ⚠️
...e-detector-aws/src/detectors/AwsEcsDetectorSync.ts 97.02% 3 Missing ⚠️
...ctor-aws/src/detectors/AwsBeanstalkDetectorSync.ts 96.00% 1 Missing ⚠️
...e-detector-aws/src/detectors/AwsEc2DetectorSync.ts 98.07% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2376      +/-   ##
==========================================
- Coverage   90.97%   90.51%   -0.46%     
==========================================
  Files         146      155       +9     
  Lines        7492     7456      -36     
  Branches     1502     1535      +33     
==========================================
- Hits         6816     6749      -67     
- Misses        676      707      +31     
Files Coverage Δ
...detector-aws/src/detectors/AwsBeanstalkDetector.ts 100.00% <100.00%> (+4.34%) ⬆️
...ource-detector-aws/src/detectors/AwsEc2Detector.ts 100.00% <100.00%> (+2.08%) ⬆️
...ource-detector-aws/src/detectors/AwsEcsDetector.ts 100.00% <100.00%> (+3.12%) ⬆️
...ource-detector-aws/src/detectors/AwsEksDetector.ts 100.00% <100.00%> (+8.75%) ⬆️
...ce-detector-aws/src/detectors/AwsLambdaDetector.ts 100.00% <100.00%> (ø)
...etector-aws/src/detectors/AwsLambdaDetectorSync.ts 100.00% <100.00%> (ø)
...metry-resource-detector-aws/src/detectors/index.ts 100.00% <100.00%> (ø)
...ctor-aws/src/detectors/AwsBeanstalkDetectorSync.ts 96.00% <96.00%> (ø)
...e-detector-aws/src/detectors/AwsEc2DetectorSync.ts 98.07% <98.07%> (ø)
...e-detector-aws/src/detectors/AwsEcsDetectorSync.ts 97.02% <97.02%> (ø)
... and 1 more

... and 69 files with indirect coverage changes

Copy link
Member

@blumamir blumamir left a comment

Choose a reason for hiding this comment

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

LGTM.

Thanks for merging all the package detectors into one PR 🙏

@david-luna
Copy link
Contributor Author

Error in hapi tests on node 18 due to a timeout :(

26 passing (2s)
  1 failing

  1) Hapi Instrumentation - Core Tests
       ESM
         should work with ESM modules:
     Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/runner/work/opentelemetry-js-contrib/opentelemetry-js-contrib/plugins/node/opentelemetry-instrumentation-hapi/test/hapi.test.ts)
      at listOnTimeout (node:internal/timers:569:17)
      at processTimers (node:internal/timers:512:7)

@david-luna david-luna merged commit 167dced into open-telemetry:main Aug 8, 2024
@dyladan dyladan mentioned this pull request Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants