-
Notifications
You must be signed in to change notification settings - Fork 792
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: add AWS Lambda detector #2102
Conversation
3530771
to
7bdb274
Compare
Codecov Report
@@ Coverage Diff @@
## main #2102 +/- ##
=======================================
Coverage 92.66% 92.67%
=======================================
Files 139 140 +1
Lines 4964 4982 +18
Branches 1026 1030 +4
=======================================
+ Hits 4600 4617 +17
- Misses 364 365 +1
|
* Returns an empty Resource if detection fails. | ||
*/ | ||
export class AwsLambdaDetector implements Detector { | ||
async detect(_config?: ResourceDetectionConfig): Promise<Resource> { |
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.
Hmm - I realized this approach doesn't work so well because in lambda we'd generally advice to use a synchronous wrapper for initializing OTel, like https://github.com/open-telemetry/opentelemetry-lambda/blob/main/nodejs/packages/layer/src/wrapper.ts
But can't await on a resource here. If I could registerInstrumentations
first to at least get code patched before an await, it'd be ok but otherwise I think code can get loaded before patch.
Any advice on a good approach? Should I just extract this into export function detectLambdaResourceSync()
?
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.
You can register instrumentations before registering the tracer factory. The instrumentations will use the global tracer factory by default which is a proxy tracer factory which returns proxy tracers which proxy calls to a noop tracer. When a tracerfactory is registered, the tracers already acquired become "active" tracers instead of noop tracers.
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.
the real solution for this problem is to make it so that resources can be appended to or similar. I know there is some disucussion around this in the spec. then you can load your sync resources at startup, then have the async ones show up when they are ready
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.
Thanks @dyladan - I came up with this pattern calling registerInstrumentations
twice
I first tried without the second registerInstrumentations
to see if tracerProvider.register()
is enough to make the tracers become "active", but it didn't seem to work. But this double-registration pattern doesn't seem too bad to me either.
If that looks ok, I think this PR is ready for review!
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.
That's strange and sounds like a bug somewhere. It should be really enough to call registerInstrumentations()
once without passing a TracerProvider
.
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.
registerInstrumentations
is needed, but the tracerProvider is optional and should be used either the one you passed or it will use the global one.
see here
https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-instrumentation/src/autoLoader.ts#L38
so just let be clear, did you try to call it registerInstrumentations
without provider and it didn't work ?
With regards to order registerInstrumentations(...)
and provider.register(...)
it shouldn't matter, both should work fine. If it doesn't work then something might be wrong with proxy or api global (just guessing)
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.
@obecny The order I did is
registerInstrumentations
withouttracerProvider
tracerProvider.register()
And it didn't seem to work yet. So I added
registerInstrumentations
withtracerProvider
I didn't try 3) registerInstrumentations
without tracerProvider
but don't really need to since I have the provider ready when doing that :)
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.
in simple setups were it is ensured that version of all components matches and there is a single place for all instrumentations to be registered,.. it's mostly don't care how this is done.
But in a more complex setup where e.g some 3rd party components are be involved which may want to install some more instrumentations,... use OTel directly,.. passing around a TracerProvider instances may easily result in problems.
Some locations use the global one, others use that one passed around.
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.
Yeah - I can try a second register without passing a provider and see how it goes. Either way I call provider.register so I guess it'll be mostly the same.
Anyways these are my wrapper in the Lambda repo, but appreciate a review of this independent code :)
I think it would be good to list the available detectors in readme of the package. Once there is such a section in readme we should also mention that the spec required attribute |
@Flarna Sounds good, added the detectors to the README |
…ry-js into aws-lambda-detector
…o aws-lambda-detector
It's not directly related to this PR. But I wonder why AWS (and GCP) detectors are in this repo instead in opentelemtry-js-contrib. |
I believe they are required by the spec at some point, but it make sense to move them back in the contrib now |
Agree they should move to contrib. I think the resource detectors actually predate the contrib repo and have just not been moved. |
just wondering which version they will have, should we do this yet before RC ? |
attributes[CLOUD_RESOURCE.REGION] = region; | ||
} | ||
|
||
// TODO(anuraaga): Migrate to FAAS_RESOURCE when defined. |
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.
// TODO(anuraaga): Migrate to FAAS_RESOURCE when defined. | |
// @TODO(anuraaga): Migrate to FAAS_RESOURCE when defined. |
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.
or perhaps you should create an issue and link it here instead of writing nickname ?
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.
Thanks, filed an issue
Versions are a problem whenever things move between. Maybe we should try independent versioning in contrib |
…o aws-lambda-detector
Thanks all, moving to contrib seems to make sense to me. Hopefully it's independent from this PR :) |
Which problem is this PR solving?
Short description of the changes