Skip to content

Add ECS and Logs attributes based on Metadata v4 endpoint#875

Merged
utpilla merged 3 commits into
open-telemetry:mainfrom
lumigo-io:ecs_detector
Mar 13, 2023
Merged

Add ECS and Logs attributes based on Metadata v4 endpoint#875
utpilla merged 3 commits into
open-telemetry:mainfrom
lumigo-io:ecs_detector

Conversation

@mmanciop

@mmanciop mmanciop commented Jan 9, 2023

Copy link
Copy Markdown
Contributor

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

linux-foundation-easycla Bot commented Jan 9, 2023

Copy link
Copy Markdown

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

utpilla commented Jan 11, 2023

Copy link
Copy Markdown
Contributor

@mmanciop Please sign the CLA.

@utpilla utpilla added the comp:extensions.aws Things related to OpenTelemetry.Extensions.AWS label Jan 11, 2023
Comment thread src/OpenTelemetry.Contrib.Extensions.AWSXRay/CHANGELOG.md
@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
Comment thread src/OpenTelemetry.Contrib.Extensions.AWSXRay/CHANGELOG.md Outdated
Comment thread src/OpenTelemetry.Contrib.Extensions.AWSXRay/Resources/AWSECSResourceDetector.cs Outdated
Comment thread src/OpenTelemetry.Contrib.Extensions.AWSXRay/Resources/AWSECSResourceDetector.cs Outdated
@mmanciop

mmanciop commented Jan 11, 2023

Copy link
Copy Markdown
Contributor Author

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

linux-foundation-easycla Bot commented Jan 11, 2023

Copy link
Copy Markdown

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

Kielek commented Jan 13, 2023

Copy link
Copy Markdown
Member

@srprash, @lupengamzn could you please check?

@github-actions github-actions Bot added the Stale label Feb 14, 2023
@github-actions

Copy link
Copy Markdown
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

Kielek commented Feb 22, 2023

Copy link
Copy Markdown
Member

@srprash?

@github-actions

github-actions Bot commented Mar 2, 2023

Copy link
Copy Markdown
Contributor

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

srprash commented Mar 2, 2023

Copy link
Copy Markdown
Contributor

@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

Kielek commented Mar 3, 2023

Copy link
Copy Markdown
Member

@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

mmanciop commented Mar 9, 2023

Copy link
Copy Markdown
Contributor Author

@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

Kielek commented Mar 9, 2023

Copy link
Copy Markdown
Member

For sure as a separate PR.

… endpoint

Co-authored-by: Piotr Kiełkowicz <pkiekowicz@splunk.com>
@mmanciop

Copy link
Copy Markdown
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 <pkiekowicz@splunk.com>
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.