feat: add AWS Lambda detector #2102

anuraaga · 2021-04-12T06:41:59Z

Which problem is this PR solving?

Support resource detection on AWS Lambda

Short description of the changes

Reads environment variables to fill resource on AWS Lambda as per Java and pending spec

codecov · 2021-04-12T07:00:25Z

Codecov Report

Merging #2102 (e85a57e) into main (d268bc6) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           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

Impacted Files	Coverage Δ
...ce-detector-aws/src/detectors/AwsLambdaDetector.ts	`100.00% <100.00%> (ø)`
...metry-resource-detector-aws/src/detectors/index.ts	`100.00% <100.00%> (ø)`
...emetry-core/src/platform/node/RandomIdGenerator.ts	`87.50% <0.00%> (-6.25%)`	⬇️

anuraaga · 2021-04-12T07:23:03Z

packages/opentelemetry-resource-detector-aws/src/detectors/AwsLambdaDetector.ts

+ * Returns an empty Resource if detection fails.
+ */
+export class AwsLambdaDetector implements Detector {
+  async detect(_config?: ResourceDetectionConfig): Promise<Resource> {


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()?

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.

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

Thanks @dyladan - I came up with this pattern calling registerInstrumentations twice

https://github.com/open-telemetry/opentelemetry-lambda/pull/30/files#diff-fc60c587485bcf624645ecb4ab13cc4393085d3f28ff0490eb275203abdf4d16R34

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!

That's strange and sounds like a bug somewhere. It should be really enough to call registerInstrumentations() once without passing a TracerProvider.

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)

@obecny The order I did is

registerInstrumentations without tracerProvider

tracerProvider.register()

And it didn't seem to work yet. So I added

registerInstrumentations with tracerProvider

I didn't try 3) registerInstrumentations without tracerProvider but don't really need to since I have the provider ready when doing that :)

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.

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 :)

Flarna · 2021-04-14T22:13:33Z

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 faas.id is currently not detected.

anuraaga · 2021-04-15T06:45:50Z

@Flarna Sounds good, added the detectors to the README

…ry-js into aws-lambda-detector

…o aws-lambda-detector

Flarna · 2021-04-15T07:25:45Z

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.

vmarchaud · 2021-04-15T08:00:03Z

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

dyladan · 2021-04-19T18:17:01Z

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.

obecny · 2021-04-19T19:19:09Z

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 ?

obecny · 2021-04-19T19:19:48Z

packages/opentelemetry-resource-detector-aws/src/detectors/AwsLambdaDetector.ts

+      attributes[CLOUD_RESOURCE.REGION] = region;
+    }
+
+    // TODO(anuraaga): Migrate to FAAS_RESOURCE when defined.


Suggested change

// TODO(anuraaga): Migrate to FAAS_RESOURCE when defined.

// @TODO(anuraaga): Migrate to FAAS_RESOURCE when defined.

or perhaps you should create an issue and link it here instead of writing nickname ?

Thanks, filed an issue

dyladan · 2021-04-19T19:48:06Z

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 ?

Versions are a problem whenever things move between. Maybe we should try independent versioning in contrib

…o aws-lambda-detector

anuraaga · 2021-04-20T02:20:28Z

Thanks all, moving to contrib seems to make sense to me. Hopefully it's independent from this PR :)

anuraaga requested review from dyladan, Flarna, johnbley, legendecas, markwolff, mayurkale22, mwear, naseemkullah, obecny, OlivierAlbertini and vmarchaud as code owners April 12, 2021 06:42

feat: add AWS Lambda detector

7bdb274

anuraaga force-pushed the aws-lambda-detector branch from 3530771 to 7bdb274 Compare April 12, 2021 06:54

anuraaga commented Apr 12, 2021

View reviewed changes

Merge branch 'main' into aws-lambda-detector

379f6a4

Anuraag Agrawal added 3 commits April 15, 2021 15:46

chore: document detectors

594b417

Merge branch 'aws-lambda-detector' of github.com:anuraaga/opentelemet…

7759f6a

…ry-js into aws-lambda-detector

Merge branch 'main' of github.com:open-telemetry/opentelemetry-js int…

d8d0b0c

…o aws-lambda-detector

Flarna approved these changes Apr 15, 2021

View reviewed changes

vmarchaud approved these changes Apr 15, 2021

View reviewed changes

dyladan approved these changes Apr 19, 2021

View reviewed changes

obecny approved these changes Apr 19, 2021

View reviewed changes

Anuraag Agrawal added 2 commits April 20, 2021 11:18

Merge branch 'main' of github.com:open-telemetry/opentelemetry-js int…

885f8de

…o aws-lambda-detector

chore: issue

bfcae45

Merge branch 'main' into aws-lambda-detector

e85a57e

vmarchaud merged commit 27f64d9 into open-telemetry:main Apr 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add AWS Lambda detector #2102

feat: add AWS Lambda detector #2102

anuraaga commented Apr 12, 2021

codecov bot commented Apr 12, 2021 •

edited

Loading

anuraaga Apr 12, 2021

dyladan Apr 12, 2021

dyladan Apr 12, 2021

anuraaga Apr 13, 2021

Flarna Apr 13, 2021

obecny Apr 13, 2021 •

edited

Loading

anuraaga Apr 13, 2021 •

edited

Loading

Flarna Apr 14, 2021

anuraaga Apr 14, 2021

Flarna commented Apr 14, 2021

anuraaga commented Apr 15, 2021

Flarna commented Apr 15, 2021

vmarchaud commented Apr 15, 2021

dyladan commented Apr 19, 2021

obecny commented Apr 19, 2021

obecny Apr 19, 2021

obecny Apr 19, 2021

anuraaga Apr 20, 2021

dyladan commented Apr 19, 2021

anuraaga commented Apr 20, 2021

	// TODO(anuraaga): Migrate to FAAS_RESOURCE when defined.
	// @TODO(anuraaga): Migrate to FAAS_RESOURCE when defined.

feat: add AWS Lambda detector #2102

feat: add AWS Lambda detector #2102

Conversation

anuraaga commented Apr 12, 2021

Which problem is this PR solving?

Short description of the changes

codecov bot commented Apr 12, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obecny Apr 13, 2021 • edited Loading

Choose a reason for hiding this comment

anuraaga Apr 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Flarna commented Apr 14, 2021

anuraaga commented Apr 15, 2021

Flarna commented Apr 15, 2021

vmarchaud commented Apr 15, 2021

dyladan commented Apr 19, 2021

obecny commented Apr 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dyladan commented Apr 19, 2021

anuraaga commented Apr 20, 2021

codecov bot commented Apr 12, 2021 •

edited

Loading

obecny Apr 13, 2021 •

edited

Loading

anuraaga Apr 13, 2021 •

edited

Loading