Skip to content
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: report url for lambda invoked via api gateway #2404

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

johnbley
Copy link
Member

Which problem is this PR solving?

  • Currently lambda invocations through an api gateway don't report the invoked url. The event passed to the handler has a different shape in that type of invocation. This can make it hard to tell invocations apart when different paths flow to the same logic.

Short description of the changes

  • I cribbed from the java implementation of this concept - basically if it appears that all the necessary information to compute the invoked url is there, then use that. Java was using the new http semantic conventions (url.full) but nothing else in this repo was, so I stuck with http.url

Copy link

codecov bot commented Aug 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.71%. Comparing base (dfb2dff) to head (5b788a2).
Report is 235 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2404      +/-   ##
==========================================
- Coverage   90.97%   90.71%   -0.27%     
==========================================
  Files         146      156      +10     
  Lines        7492     7659     +167     
  Branches     1502     1585      +83     
==========================================
+ Hits         6816     6948     +132     
- Misses        676      711      +35     
Files with missing lines Coverage Δ
...-instrumentation-aws-lambda/src/instrumentation.ts 95.81% <100.00%> (+2.13%) ⬆️

... and 75 files with indirect coverage changes

@pichlermarc pichlermarc added the pkg-status:unmaintained This package is unmaintained. Only bugfixes may be acceped until a new owner has been found. label Sep 4, 2024
Copy link
Contributor

github-actions bot commented Sep 5, 2024

This package does not have an assigned component owner and is considered unmaintained. As such this package is in feature-freeze and this PR will be closed with 14 days unless a new owner or a sponsor (a member of @open-telemetry/javascript-approvers) for the feature is found. It is the responsibility of the author to find a sponsor for this feature.
Are you familiar with this package? Consider becoming a component owner.

@johnbley
Copy link
Member Author

johnbley commented Sep 6, 2024

Could we mark @open-telemetry/lambda-extension-approvers or somesuch as the owner of the lambda instrumentation? Essentially this component is used as part of the otel-lambda repo/build.

@johnbley
Copy link
Member Author

johnbley commented Sep 6, 2024

Could we mark @open-telemetry/lambda-extension-approvers or somesuch as the owner of the lambda instrumentation? Essentially this component is used as part of the otel-lambda repo/build.

@pichlermarc also #2403 was approved and merged a couple days ago... this lambda instrumentation is in fact under active development/maintenance.

@tylerbenson your thoughts on lambda sig ownership of the various language components?

@tylerbenson
Copy link
Member

I'm fine with the lambda approvers taking ownership. @serkan-ozal also volunteered to own it if you'd prefer an individual vs a team.

@pichlermarc
Copy link
Member

Could we mark @open-telemetry/lambda-extension-approvers or somesuch as the owner of the lambda instrumentation? Essentially this component is used as part of the otel-lambda repo/build.

@johnbley absolutely, we'd very much appreciate this. 🙂 The way to do is is that someone from @open-telemetry/lambda-extension-maintainers opens a component ownership request issue. I'll then open a PR to add the team there 🙂

@pichlermarc also #2403 was approved and merged a couple days ago... this lambda instrumentation is in fact under active development/maintenance.

In #2403 @dyladan acted as a feature sponsor according to the guidelines. Components are marked as "unmaintained" when there's no assigned owner.

Unfortunately, we had to introduce this guideline as many contributed components (like @opentelemetry/instrumentation-aws-lambda) got abandoned by their original owners. We would like to keep all components around, but the diverse set of packages that are being instrumented in this repo require immense maintenance effort which significantly slows down SDK/API development.

@johnbley
Copy link
Member Author

@serkan-ozal Thanks for your careful review!

@serkan-ozal
Copy link
Contributor

@johnbley You're welcome and thanks for the contribution.

@david-luna
Copy link
Contributor

there is a ongoing PR for adding a component owner #2506

@jj22ee you may want to have a look a this one

'x-forwarded-proto': 'http',
},
queryStringParameters: {
key: 'value',
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add another key/value to test the & addition logic from _extractFullUrl?

describe('url parsing', () => {
it('pulls url from api gateway rest events', async () => {
initializeHandler('lambda-test/sync.handler');
const event = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did a quick check via logging the event from REST APIGW ---> Lambda scenario (enabled Lambda proxy integration)

The capitalization for some event fields from REST APIGW are a bit different than from HTTP APIGW. (This doc also verifies some of the capitalization diffs for REST APIGW events
)

  • Sampled REST APIGW event:
{
  ...
  path: '/',
  ...
  headers: {
    ...
    Host: '<REST_API>.execute-api.us-west-2.amazonaws.com',
    ...
    'X-Forwarded-For': '15.248.7.4',
    'X-Forwarded-Port': '443',
    'X-Forwarded-Proto': 'https'
  },
  ...
  queryStringParameters: { ghd: '356', jyi: '4568' },
  ...
}
  • Sampled HTTP APIGW event
{
  ...
  rawPath: '/lambda1/route2',
  ...
  headers: {
    ...
    host: '<HTTP_API>.execute-api.us-west-2.amazonaws.com',
    ...
    'x-forwarded-for': '15.248.7.4',
    'x-forwarded-port': '443',
    'x-forwarded-proto': 'https'
  },
  queryStringParameters: { abc: '123', def: '456' },
  ...
}

Copy link
Contributor

Choose a reason for hiding this comment

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

To support REST APIGW event, can you update the URL extraction to also account for Host, X-Forwarded-Port, and X-Forwarded-Proto, and then update the REST event test?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:instrumentation-aws-lambda pkg-status:unmaintained:autoclose-scheduled pkg-status:unmaintained This package is unmaintained. Only bugfixes may be acceped until a new owner has been found.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants