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

Add HTTP attributes support for AWS lambda function wrapper #1780

Conversation

mateuszrzeszutek
Copy link
Member

Related to #1773

This PR implements HTTP server spec for lambdas that use APIGatewayProxyRequestEvent as input.

import java.nio.charset.StandardCharsets;
import java.util.Map;

final class HttpSpanAttributes {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think it makes sense to have a AwsLambdaGatewayProxyTracer extends HttpServerTracer?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd considered choosing different tracers based on the input class, but AwsLambdaTracer has some AWS specific logic (context propagation, faas.* attributes) that I'd need to duplicate in a new tracer (which would extend HttpServerTracer); I'd have to put the AWS logic in a separate helper class if I were to do that.
So out of the two choices: AWS logic in tracer vs HTTP logic in tracer I'd decided to put the HTTP things in the second place; it seemed to me that it'd be easier to add support for other event types this way (share the same tracer class with all the AWS things, separate strategy for additional attributes)

@mateuszrzeszutek mateuszrzeszutek force-pushed the support-http-attributes-in-aws-lambda branch from 5504488 to ebc5ab8 Compare November 30, 2020 11:25
@iNikem
Copy link
Contributor

iNikem commented Dec 2, 2020

@anuraaga nobody review this but you :) Or should I just merge it?

Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Oops sorry for missing - I'm a bit worried if the maintenance or http conventions is harder with this approach, e.g., when adding new http sem conventions. But I think it's fine for now anyways

@iNikem iNikem merged commit fff6eb7 into open-telemetry:master Dec 3, 2020
@mateuszrzeszutek mateuszrzeszutek deleted the support-http-attributes-in-aws-lambda branch February 5, 2021 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants