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

Support output value in AttributeSetter #6543

Merged

Conversation

SodaDev
Copy link
Contributor

@SodaDev SodaDev commented Jan 2, 2025

Reasoning:
Add AttributeBuilder, which will extend the AttributeSetter contract with output value. Thanks to that, users can instrument attributes based on the AWS SDK responses.

@SodaDev SodaDev requested a review from a team as a code owner January 2, 2025 15:06
Copy link

linux-foundation-easycla bot commented Jan 2, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

@github-actions github-actions bot requested a review from akats7 January 2, 2025 15:06
@SodaDev SodaDev force-pushed the support-output-in-attributesetter branch 4 times, most recently from ae8c6cb to a976877 Compare January 2, 2025 15:19
Copy link
Member

@dmathieu dmathieu left a comment

Choose a reason for hiding this comment

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

This will need a changelog entry.

Copy link

codecov bot commented Jan 6, 2025

Codecov Report

Attention: Patch coverage is 88.88889% with 3 lines in your changes missing coverage. Please review.

Project coverage is 73.0%. Comparing base (7d7606c) to head (fa17b57).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...github.com/aws/aws-sdk-go-v2/otelaws/attributes.go 66.6% 1 Missing ⚠️
...hub.com/aws/aws-sdk-go-v2/otelaws/snsattributes.go 50.0% 1 Missing ⚠️
...hub.com/aws/aws-sdk-go-v2/otelaws/sqsattributes.go 50.0% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #6543   +/-   ##
=====================================
  Coverage   73.0%   73.0%           
=====================================
  Files        189     189           
  Lines      15754   15773   +19     
=====================================
+ Hits       11503   11519   +16     
- Misses      3916    3920    +4     
+ Partials     335     334    -1     
Files with missing lines Coverage Δ
...tation/github.com/aws/aws-sdk-go-v2/otelaws/aws.go 95.8% <100.0%> (+0.1%) ⬆️
...ion/github.com/aws/aws-sdk-go-v2/otelaws/config.go 100.0% <100.0%> (ø)
...om/aws/aws-sdk-go-v2/otelaws/dynamodbattributes.go 100.0% <100.0%> (ø)
...github.com/aws/aws-sdk-go-v2/otelaws/attributes.go 89.4% <66.6%> (+7.1%) ⬆️
...hub.com/aws/aws-sdk-go-v2/otelaws/snsattributes.go 81.4% <50.0%> (-6.6%) ⬇️
...hub.com/aws/aws-sdk-go-v2/otelaws/sqsattributes.go 94.4% <50.0%> (-5.6%) ⬇️

@SodaDev SodaDev force-pushed the support-output-in-attributesetter branch from a976877 to e8d0895 Compare January 6, 2025 12:53
…with output value. Thanks to that, users can instrument attributes based on the AWS SDK responses.
@SodaDev SodaDev force-pushed the support-output-in-attributesetter branch from e8d0895 to 6e05dfc Compare January 6, 2025 15:44
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Damien Mathieu <[email protected]>
@SodaDev
Copy link
Contributor Author

SodaDev commented Jan 19, 2025

@dmathieu @akats7 I will never merge that PR. Every time I resolve conflicts, checks are blocked and have to be approved, and after they are approved, there is a new merge to the main, and again, I can't merge it because of a conflict. Could you help me?

Could you merge it after the checks pass? I am not getting any notifications after your approval; the checks are green.

@dmathieu
Copy link
Member

Ping @open-telemetry/go-approvers for second review.

@SodaDev SodaDev requested a review from akats7 January 22, 2025 07:50
@SodaDev SodaDev force-pushed the support-output-in-attributesetter branch from 8a9ab09 to cb3594a Compare January 22, 2025 17:47
Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

Please sync with main and address the comment.

I plan to approve and merge this on Friday given all comments are addressed.

@SodaDev SodaDev force-pushed the support-output-in-attributesetter branch from cb3594a to d37aea5 Compare January 22, 2025 19:00
Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

Planning to merge this on Friday.

@SodaDev, thank you for your contribution 🏅

CHANGELOG.md Outdated Show resolved Hide resolved
@pellared
Copy link
Member

pellared commented Jan 24, 2025

@dmathieu, can you make a double-check and merge?

@dmathieu dmathieu merged commit 130846a into open-telemetry:main Jan 24, 2025
26 checks passed
@pellared
Copy link
Member

@SodaDev, thank you for your contribution 🏅

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.

4 participants