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

NH-89194: upgrade to upstream 2.8.0 #262

Merged
merged 1 commit into from
Sep 20, 2024
Merged

NH-89194: upgrade to upstream 2.8.0 #262

merged 1 commit into from
Sep 20, 2024

Conversation

cleverchuk
Copy link
Contributor

@cleverchuk cleverchuk commented Aug 30, 2024

Upgrades to upstream 2.8.0.

@cleverchuk cleverchuk requested a review from a team as a code owner August 30, 2024 18:37
cheempz
cheempz previously approved these changes Aug 30, 2024
Copy link
Contributor

@cheempz cheempz left a comment

Choose a reason for hiding this comment

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

LGTM, would be good to confirm the upstream improvement for lambda instrumentation with our layer!

@cleverchuk
Copy link
Contributor Author

LGTM, would be good to confirm the upstream improvement for lambda instrumentation with our layer!

@cheempz Any specific item you're thinking of?

@cleverchuk cleverchuk changed the title NH-89194: upgrade to upstream 2.7.0 NH-89194: upgrade to upstream 2.8.0 Sep 17, 2024
@cleverchuk cleverchuk requested a review from cheempz September 17, 2024 18:48
@cheempz
Copy link
Contributor

cheempz commented Sep 18, 2024

LGTM, would be good to confirm the upstream improvement for lambda instrumentation with our layer!

@cheempz Any specific item you're thinking of?

I'm thinking some checks specifically around a function with the RequestStreamHandler interface, where our layer should reflect the upstream fix... hmm and maybe that a function with RequestHandler interface still works as expected, no regression.

Copy link
Contributor

@cheempz cheempz left a comment

Choose a reason for hiding this comment

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

👍

@cleverchuk
Copy link
Contributor Author

It looks like there isn't any changes affecting those components in the release?

@cheempz
Copy link
Contributor

cheempz commented Sep 18, 2024

It looks like there isn't any changes affecting those components in the release?

i was looking at open-telemetry/opentelemetry-java-instrumentation#11868 which is part of the upstream 2.7.0 release, it seems to have changed the lambda instrumentation.

@cleverchuk
Copy link
Contributor Author

i was looking at open-telemetry/opentelemetry-java-instrumentation#11868 which is part of the upstream 2.7.0 release, it seems to have changed the lambda instrumentation.

Gotcha. It looks like that only affects users that implement those abstract classes instead of the vanilla lambda ones or uses the wrapper for loading their code.

@cleverchuk
Copy link
Contributor Author

cleverchuk commented Sep 19, 2024

Looking at the code again, I'm realizing to actually capture different lambda events object attributes like APIGatewayProxyRequestEvent http headers, the user will need to use one of these wrappers for the event they're targeting. It explains why the generic one doesn't do anything other than generating the span.
Not true, they use the similar code path. It looks like context propagation will only work with the wrapper version.

@cleverchuk cleverchuk merged commit 1063e26 into main Sep 20, 2024
13 of 14 checks passed
@cleverchuk cleverchuk deleted the cc/NH-89194 branch September 20, 2024 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants