[Instrumentation.AWSLambda] Issue/aws lambda http server semantic conventions attributes

[rypdal]

Changes

Added HTTP Server semantic conventions required tags to Activity created in OpenTelemetry.Instrumentation.AWSLambda.

[codecov]

Codecov Report

Merging #626 (b26223b) into main (59dbc5d) will increase coverage by 0.36%.
The diff coverage is 94.94%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #626      +/-   ##
==========================================
+ Coverage   77.22%   77.59%   +0.36%     
==========================================
  Files         173      175       +2     
  Lines        5160     5222      +62     
==========================================
+ Hits         3985     4052      +67     
+ Misses       1175     1170       -5     

Impacted Files	Coverage Δ
...ntation.AWSLambda/Implementation/AWSLambdaUtils.cs	88.05% <80.00%> (+9.79%)	⬆️
...ation.AWSLambda/Implementation/CommonExtensions.cs	80.00% <80.00%> (ø)
...etry.Instrumentation.AWSLambda/AWSLambdaWrapper.cs	94.73% <96.55%> (+0.39%)	⬆️
...ion.AWSLambda/Implementation/AWSLambdaHttpUtils.cs	100.00% <100.00%> (ø)

[Oberon00]

Hi, I would propose we merge this only after the first release is cut in #590.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[rypdal]

This PR was waiting for the next -beta release. The release was published and PR can be further reviewed and merged.

[rypdal]

I think this now looks mostly done, only a few nitpicks left. 👍 Thank you for addressing my comments!

@Oberon00, thank you for the review !

[Oberon00]

We need make sure the URL-encoding of the query string parts is correct, see
#626 (comment)

[rypdal]

Hi, I would propose we merge this only after the first release is cut in #590.

Hi @utpilla , the corresponding release already happened and this PR can be merged. The PR requires one more approval.

[utpilla]

@rypdal Update the CHANGELOG.

[rypdal]

@rypdal Update the CHANGELOG.

@utpilla - done. Thanks !

[rypdal]

I made small code adaptation to be aligned with the AWS doc (see request examples https://docs.aws.amazon.com/apigateway/latest/developerguide/http-api-develop-integrations-lambda.html):

• use the last instead of the first value of multi-value headers (this can be seen from the example)
• added respective unit tests
• made logic for setting http.target more precise to exclude some edge case: this property shouldn't be set if neither path nor query string is provided

These changes shouldn't be obstacles for merge.

[Oberon00]

this property shouldn't be set if neither path nor query string is provided

Is this possible? I don't think that would be a HTTP request then. Maybe we should fall back to / or empty string in this case?

[Oberon00]

I think I would prefer always treating Path as if it was empty if it is actually null. But do not add extra code to try to get something sensible in these situations; just ensure we do not crash. Garbage-in/garbage-out applies here. (BTW: Before the next release we should ensure we catch any errors that happen before the traced handler is called and call it without tracing in the worst case; the code is now complex enough for a good chance of bugs)

[rypdal]

I think I would prefer always treating Path as if it was empty if it is actually null. But do not add extra code to try to get something sensible in these situations; just ensure we do not crash. Garbage-in/garbage-out applies here. (BTW: Before the next release we should ensure we catch any errors that happen before the traced handler is called and call it without tracing in the worst case; the code is now complex enough for a good chance of bugs)

this property shouldn't be set if neither path nor query string is provided

Is this possible? I don't think that would be a HTTP request then. Maybe we should fall back to / or empty string in this case?

@Oberon00 : I rolled back to the previous code and updated unit test. So, even if neither path nor query string is set the http.target is still set to an empty string.

[rypdal]

Seems build fails because of failing test not related to changes from this PR:

Failed OpenTelemetry.Instrumentation.Wcf.Tests.TelemetryClientMessageInspectorTests.OutgoingRequestInstrumentationTest(instrument: True, filter: False, suppressDownstreamInstrumentation: True, includeVersion: True, enrich: False, enrichmentException: False, emptyOrNullAction: False) [1 ms]
Error Message:
System.ObjectDisposedException : Cannot access a disposed object.

Details can be found here: https://github.com/open-telemetry/opentelemetry-dotnet-contrib/actions/runs/3188299150/jobs/5200797125

[Oberon00]

I think this PR can be merged now. (The build failure was indeed unrelated, it became green after changing the totally unrelated CHANGELOG)
CC @utpilla