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 ECS and Logs attributes based on Metadata v4 endpoint #875

Merged
merged 3 commits into from
Mar 13, 2023

Conversation

mmanciop
Copy link
Contributor

@mmanciop mmanciop commented Jan 9, 2023

Changes

Implement aws.ecs and aws.log resource attributes with data from the ECS Metadata endpoint v4.

  • Appropriate CHANGELOG.md updated for non-trivial changes

@mmanciop mmanciop requested a review from a team January 9, 2023 12:39
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 9, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@mmanciop mmanciop marked this pull request as draft January 9, 2023 12:39
@utpilla
Copy link
Contributor

utpilla commented Jan 11, 2023

@mmanciop Please sign the CLA.

@utpilla utpilla added the comp:extensions.aws Things related to OpenTelemetry.Extensions.AWS label Jan 11, 2023
@mmanciop mmanciop marked this pull request as ready for review January 11, 2023 12:18
@mmanciop mmanciop force-pushed the ecs_detector branch 3 times, most recently from e7430ae to 49ac4d3 Compare January 11, 2023 14:53
@mmanciop
Copy link
Contributor Author

mmanciop commented Jan 11, 2023

I am actually not 100% sure yet of the patchset wrt the implementation of the IResourceDetector interface. I am going to try it out on a test app.

Update: It seems to me we need to get rid of the local IResourceDetector if we want this to work with OpenTelemetry SDK 1.3.0+.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 11, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@mmanciop mmanciop force-pushed the ecs_detector branch 3 times, most recently from 3c5403f to 4d189d8 Compare January 11, 2023 22:16
@mmanciop mmanciop requested a review from Kielek January 11, 2023 22:22
@Kielek Kielek requested a review from srprash January 13, 2023 06:43
@Kielek
Copy link
Contributor

Kielek commented Jan 13, 2023

@srprash, @lupengamzn could you please check?

@github-actions github-actions bot added the Stale label Feb 14, 2023
@github-actions
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Feb 22, 2023
@Kielek Kielek reopened this Feb 22, 2023
@Kielek Kielek removed the Stale label Feb 22, 2023
@Kielek
Copy link
Contributor

Kielek commented Feb 22, 2023

@srprash?

@github-actions
Copy link
Contributor

github-actions bot commented Mar 2, 2023

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

@github-actions github-actions bot added the Stale label Mar 2, 2023
@srprash
Copy link
Contributor

srprash commented Mar 2, 2023

@srprash, @lupengamzn could you please check?

Would you review? And also comment on the breaking change/major version bump as well?

I agree that this must go out as a major version bump.

Just a reminder, in case of major bump, the whole package should be renamed. At least .Contrib infix has to be removed.

Yup. We can do that to be inline with the rest of the packages in the repo. I wish we can do the same for the OpenTelemetry.Contrib.Instrumentation.AWS package as well while bumping its major version.

If renaming the package is being considered, could we please replace XRay with AWS in the name?

I have no issues in dropping XRay and renaming the package to be OpenTelemetry.Contrib.Extensions.AWS. We had it with XRay because initially the package contained only the id-generator and the propagator very specific to AWS XRay.

@github-actions github-actions bot removed the Stale label Mar 3, 2023
@Kielek
Copy link
Contributor

Kielek commented Mar 3, 2023

@mmanciop, finally we have an approval from the package owner.
Could you please solve the conflicts? Then we should be able to make final review and merge.

@mmanciop
Copy link
Contributor Author

mmanciop commented Mar 9, 2023

@Kielek @srprash I solved the conflicts and did some cleanup, but I am assuming that version bumping, extension renaming and the rest mentioned in #875 (comment) will be performed by the package owners, right?

@Kielek
Copy link
Contributor

Kielek commented Mar 9, 2023

For sure as a separate PR.

@mmanciop
Copy link
Contributor Author

@mmanciop, finally we have an approval from the package owner. Could you please solve the conflicts? Then we should be able to make final review and merge.

Now that I found out why PublicApiAnalyzers did not run in my setup (#1071), I dearly hope this commit is it :-)

Co-authored-by: Piotr Kiełkowicz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:extensions.aws Things related to OpenTelemetry.Extensions.AWS
Projects
None yet
Development

Successfully merging this pull request may close these issues.