-
Notifications
You must be signed in to change notification settings - Fork 545
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
feat(detector-aws): add DetectorSync implementation for Lambda detector #2329
feat(detector-aws): add DetectorSync implementation for Lambda detector #2329
Conversation
…ce for lambda detector
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2329 +/- ##
==========================================
- Coverage 90.97% 90.41% -0.57%
==========================================
Files 146 150 +4
Lines 7492 7367 -125
Branches 1502 1527 +25
==========================================
- Hits 6816 6661 -155
- Misses 676 706 +30
|
@@ -235,7 +235,11 @@ function getDisabledInstrumentationsFromEnv() { | |||
export function getResourceDetectorsFromEnv(): Array<Detector | DetectorSync> { | |||
const resourceDetectors = new Map< | |||
string, | |||
Detector | DetectorSync | Detector[] | DetectorSync[] | |||
| Detector |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note for reviewer: this is necessary to allow this single refactor instead of doing all AWS detector at once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not do all the AWS detectors at once? What is the gain in opening so many PRs with the exact same refactor?
I personally would prefer to have all detectors in the repo updated in one PR, but at least the ones from the same npm package can be grouped together, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not do all the AWS detectors at once? What is the gain in opening so many PRs with the exact same refactor?
I personally would prefer...
@blumamir as you said is more a preference although I find some benefits of splitting. We can discuss each detector independently and allow other detectors to be merged in case one discussion takes long time.
it's my preference but I'm fine in adding up all the changes in a single big PR. I preferred that way to let the owners
The @opentelemetry/resources version requirement in the package.json needs to be updated as ^1.10.0. |
…erface
closed in favour of #2376 following this suggestion |
Context
Another PR moving away from the deprecated
Detector
interface in favour to theDetectorSync
interface. This time is for AWS Lambda detector and it might help a bit in open-telemetry/opentelemetry-js#3582Not doing all detectors at once:
Short description of the changes
AwsLambdaDetectorSync
class and instanceAwsLambdaDetector
reuse the new sync instanceNote: since this package is marked as stable (major version is 1) we are doing a non breaking change by adding the new class and mark the current as
deprecated
. The async detector changes behavior, it does not throw anymore, and it was discussed in last SIG meeting (2024-07-31). The outcome was we're okay with that.A couple of PRs will follow: