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

[Instrumentation.AwsLambda] Rename Trace to TraceAsync where appropriate #608

Conversation

Oberon00
Copy link
Member

@Oberon00 Oberon00 commented Aug 24, 2022

Sorry for another renaming proposal, but it seems really bad that we have methods that you cannot await and methods that you need to await with the same name. Also, the overload resolution between some of them could get tricky.

Note: The diff looks really strange here, please look at the two commits separately to see what's actually going on.

@Oberon00 Oberon00 force-pushed the feature/awslambda-traceasync branch from 5dd1980 to d684283 Compare August 24, 2022 16:25
@Oberon00 Oberon00 force-pushed the feature/awslambda-traceasync branch from d684283 to 216ea99 Compare August 24, 2022 16:29
@codecov
Copy link

codecov bot commented Aug 24, 2022

Codecov Report

Merging #608 (a2dd888) into main (e7bc1b0) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #608   +/-   ##
=======================================
  Coverage   76.70%   76.70%           
=======================================
  Files         170      170           
  Lines        5161     5161           
=======================================
  Hits         3959     3959           
  Misses       1202     1202           
Impacted Files Coverage Δ
...etry.Instrumentation.AWSLambda/AWSLambdaWrapper.cs 94.82% <100.00%> (ø)

@utpilla utpilla added the comp:instrumentation.aws Things related to OpenTelemetry.Instrumentation.AWS label Aug 24, 2022
@Oberon00 Oberon00 marked this pull request as ready for review August 25, 2022 12:26
@Oberon00 Oberon00 requested a review from a team August 25, 2022 12:26
@@ -89,80 +89,80 @@ public class AWSLambdaWrapper
}

/// <summary>
/// Tracing wrapper for async Lambda handler.
/// Tracing wrapper for Lambda handler.
Copy link
Member Author

Choose a reason for hiding this comment

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

Please look at the commits in the diff separately, I had to move a function around and that causes to the diffing algorithm to go off the rails apparently.

@utpilla
Copy link
Contributor

utpilla commented Aug 25, 2022

@rypdal Could you please review this PR?

@utpilla utpilla added comp:instrumentation.awslambda Things related to OpenTelemetry.Instrumentation.AWSLambda and removed comp:instrumentation.aws Things related to OpenTelemetry.Instrumentation.AWS labels Aug 25, 2022
@Oberon00
Copy link
Member Author

@rypdal Will probably not be available before Thursday.

Copy link
Contributor

@rypdal rypdal left a comment

Choose a reason for hiding this comment

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

Changes are looking good and can be merged.

@utpilla
Copy link
Contributor

utpilla commented Sep 1, 2022

Please update the branch with the latest changes in main and we can merge this PR. I do not have the permissions to update your branch.

@Oberon00
Copy link
Member Author

Oberon00 commented Sep 2, 2022

@utpilla I think I enabled maintainer access now.

@cijothomas cijothomas merged commit 071a1d9 into open-telemetry:main Sep 2, 2022
@Oberon00 Oberon00 deleted the feature/awslambda-traceasync branch September 5, 2022 07:50
@Oberon00
Copy link
Member Author

Oberon00 commented Sep 6, 2022

Forgot the changelog entry, will add it to the release PR #590.

Oberon00 added a commit to dynatrace-oss-contrib/opentelemetry-dotnet-contrib that referenced this pull request Sep 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:instrumentation.awslambda Things related to OpenTelemetry.Instrumentation.AWSLambda
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants