feat: allow custom aws sdk service extensions via config (#829)

[chrisrichardsevergreen]

Which problem is this PR solving?

Implements #829 - allowing the end user to provide custom service extensions, meaning trace semantic conventions can be added by the user to a much larger array of AWS services that could be reasonably expected to be built into this package. It also also for users to 'tweak' the semantics of existing service specific logic.

Short description of the changes

• Add config parameter customServiceExtensions, and merge these into the existing service extensions (as implemented this allows overriding the built-in ones, however this is definitely open for debate!)
• Expose the ServiceExtension type and dependent types on the package interface, as well as the service specific types so these can be built upon
• Add tests to the handling of these custom service extensions

Checklist

• [ x ] Ran npm run test-all-versions for the edited package(s) on the latest commit if applicable.

[codecov]

Codecov Report

Merging #877 (dc1a360) into main (47700e1) will increase coverage by 0.37%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #877      +/-   ##
==========================================
+ Coverage   95.29%   95.66%   +0.37%     
==========================================
  Files          10       20      +10     
  Lines         701     1199     +498     
  Branches      142      245     +103     
==========================================
+ Hits          668     1147     +479     
- Misses         33       52      +19     

Impacted Files	Coverage Δ
...tapackages/auto-instrumentations-node/src/utils.ts	97.87% <0.00%> (ø)
...opentelemetry-instrumentation-aws-sdk/src/utils.ts	97.36% <0.00%> (ø)
...etry-instrumentation-aws-sdk/src/services/index.ts	100.00% <0.00%> (ø)
...entelemetry-instrumentation-aws-sdk/src/aws-sdk.ts	97.44% <0.00%> (ø)
...emetry-instrumentation-aws-sdk/src/services/sqs.ts	89.65% <0.00%> (ø)
...opentelemetry-instrumentation-aws-sdk/src/enums.ts	100.00% <0.00%> (ø)
...y-instrumentation-aws-sdk/src/services/dynamodb.ts	100.00% <0.00%> (ø)
...entation-aws-sdk/src/services/MessageAttributes.ts	90.32% <0.00%> (ø)
...emetry-instrumentation-aws-sdk/src/services/sns.ts	93.75% <0.00%> (ø)
...ntation-aws-sdk/src/services/ServicesExtensions.ts	100.00% <0.00%> (ø)

[blumamir]

The existing hooks are meant exactly for that:

| `preRequestHook`                  | `AwsSdkRequestCustomAttributeFunction`    | Hook called before request send, which allow to add custom attributes to span.                                             |
| `responseHook`                    | `AwsSdkResponseCustomAttributeFunction`   | Hook for adding custom attributes when response is received from aws.          

What is the motivation to add a second mechanism?

[chrisrichardsevergreen]

My understanding was that these didn't allow attributes to be added to the AWS SDK parameters though, so those existing mechanisms wouldn't allow custom behaviour in terms of trace propagation via e.g. ClientContext on lambda invoke, the only way to achieve this would be to add utility methods in client code, however it felt cleaner to be able to configure the instrumentation to do this for you.

[blumamir]

My understanding was that these didn't allow attributes to be added to the AWS SDK parameters though, so those existing mechanisms wouldn't allow custom behaviour in terms of trace propagation via e.g. ClientContext on lambda invoke, the only way to achieve this would be to add utility methods in client code, however it felt cleaner to be able to configure the instrumentation to do this for you. Get Outlook for Androidhttps://aka.ms/AAb9ysg
…
________________________________ From: Amir Blum @.> Sent: Sunday, February 13, 2022 4:33:18 PM To: open-telemetry/opentelemetry-js-contrib @.> Cc: Chris Richards @.>; Author @.> Subject: Re: [open-telemetry/opentelemetry-js-contrib] feat: allow custom aws sdk service extensions via config (#829) (PR #877) [EXTERNAL] The existing hooks are meant exactly for that: | preRequestHook | AwsSdkRequestCustomAttributeFunction | Hook called before request send, which allow to add custom attributes to span. | | responseHook | AwsSdkResponseCustomAttributeFunction | Hook for adding custom attributes when response is received from aws. What is the motivation to add a second mechanism? — Reply to this email directly, view it on GitHubhttps://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fopen-telemetry%2Fopentelemetry-js-contrib%2Fpull%2F877%23issuecomment-1038241683&data=04%7C01%7Cchris.richards%40evergreenenergy.co.uk%7C2e6e8b151fac4cb0804508d9ef0e8b20%7C4a8a291b68334d938bd1b21a97d084d4%7C0%7C0%7C637803668009322544%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=zlg%2FoxVfF5kL0K2ex%2FRI5Og51DhPqIRHIdZpkKlf2Yc%3D&reserved=0, or unsubscribehttps://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAW2EGJBUP6SXCTEFPCPGW5TU27MM5ANCNFSM5OAJPRDA&data=04%7C01%7Cchris.richards%40evergreenenergy.co.uk%7C2e6e8b151fac4cb0804508d9ef0e8b20%7C4a8a291b68334d938bd1b21a97d084d4%7C0%7C0%7C637803668009322544%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=nqYdMSqtU9WYNvNuAx6iwNNM%2Foa7JyT1MWO8dngWQ90%3D&reserved=0. Triage notifications on the go with GitHub Mobile for iOShttps://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fapps.apple.com%2Fapp%2Fapple-store%2Fid1477376905%3Fct%3Dnotification-email%26mt%3D8%26pt%3D524675&data=04%7C01%7Cchris.richards%40evergreenenergy.co.uk%7C2e6e8b151fac4cb0804508d9ef0e8b20%7C4a8a291b68334d938bd1b21a97d084d4%7C0%7C0%7C637803668009478798%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=IeEOMe3TlLj4dG2X4Pdo9GpYenTnG%2Bqf43mkxKThMWw%3D&reserved=0 or Androidhttps://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fplay.google.com%2Fstore%2Fapps%2Fdetails%3Fid%3Dcom.github.android%26referrer%3Dutm_campaign%253Dnotification-email%2526utm_medium%253Demail%2526utm_source%253Dgithub&data=04%7C01%7Cchris.richards%40evergreenenergy.co.uk%7C2e6e8b151fac4cb0804508d9ef0e8b20%7C4a8a291b68334d938bd1b21a97d084d4%7C0%7C0%7C637803668009478798%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=JF%2BYKLsPBbBHh1VOGBIf%2FABvtvtaHxoVNlxBSzDtbjI%3D&reserved=0. You are receiving this because you authored the thread.Message ID: @.***>

We can extend the existing services built into the instrumentation to support more services according to semantic conventions, or extend the existing hooks to expose more parameters.

Can you supply an example of how you plan to use this new config option? and why it cannot go into the instrumentation code directly?

[chrisrichardsevergreen]

LambdaTestCustomServiceExtension in the unit tests is a fairly close representation of the sort of thing I had in mind, particularly the requestPostSpanHook. More than happy for this to go in as built in functionality if you'd be receptive to that as a PR instead.

[blumamir]

LambdaTestCustomServiceExtension in the unit tests is a fairly close representation of the sort of thing I had in mind, particularly the requestPostSpanHook. More than happy for this to go in as built in functionality if you'd be receptive to that as a PR instead.

Sorry I still don't understand. What is LambdaTest and which unit tests are you referring to?

Please share:

• The name of the service you are trying to instrument
• The operations from this service which you invoke
• Which semantic attributes do you need to add to the span
• What modification are you planning to execute on the request
  Thanks

[chrisrichardsevergreen]

Sorry I still don't understand. What is LambdaTest and which unit tests are you referring to?

Please share:

• The name of the service you are trying to instrument
• The operations from this service which you invoke
• Which semantic attributes do you need to add to the span
• What modification are you planning to execute on the request
  Thanks

I was referring to LambdaTestCustomServiceExtension, added to test fixtures as part of this PR - we'd have likely used almost exactly that as a custom extension if this PR had been accepted.

However:

• I'm trying to instrument Lambda
• I wanted to instrument Invoke and InvokeAsync on this
• There aren't any semantic attributes missing from the span if I'm honest as the AWS SDK already adds a fair number, although from the semantic convention spec there did seem to be some useful ones on the response that I added, although this was primarily driven by a need to ensure all hooks were tested rather than a sense that we were missing anything.
• As the fixture shows the request modification I wanted to make was injecting the trace-context into the Custom field on the ClientContext parameter of the request, as this is available as a dictionary in the lambda. The reason I felt extension hooks rather than built-in functionality might be more appropriate here was this didn't feel like a one-size fits all solution

If anything this PR was a strawman solution for issue #829, currently assigned to @willarmiros, so more than happy for alternative suggestions

[blumamir]

I was referring to LambdaTestCustomServiceExtension, added to test fixtures as part of this PR - we'd have likely used almost exactly that as a custom extension if this PR had been accepted.

Makes sense, I missed that :) thanks

I totally support adding this logic directly into the instrumentation code. The current list of supported services is very partial and is meant to be extended as needed. Context injection and FAAS attributes will probably be very useful to all users.
@willarmiros @NathanielRN - do you agree?

[willarmiros]

Yep, seems like a reasonable additive change!

[blumamir]

@chrisrichardsevergreen - Are you good with this suggested change?
Do you want to implement it in this PR? or close the current one and open a new one?

[chrisrichardsevergreen]

@chrisrichardsevergreen - Are you good with this suggested change? Do you want to implement it in this PR? or close the current one and open a new one?

Hi - just in the process of preparing a PR for the change. It's probably easier to create a new PR for the change as it's so different. It'll probably be ready at some point this week.

[chrisrichardsevergreen]

@chrisrichardsevergreen - Are you good with this suggested change? Do you want to implement it in this PR? or close the current one and open a new one?

Hi - just in the process of preparing a PR for the change. It's probably easier to create a new PR for the change as it's so different. It'll probably be ready at some point this week.

@blumamir I've now raised #916 with this change