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.

Sign up for GitHub

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

Jump to bottom

#3308 Support adding container.id to resource metadata #3321

Merged
merged 14 commits into from
Jul 21, 2021
Merged

#3308 Support adding container.id to resource metadata #3321

merged 14 commits into from
Jul 21, 2021

Conversation

lo-jason
Copy link
Contributor

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 16, 2021 •
edited
Loading

CLA Signed

The committers are authorized under a signed CLA.

@jkwatson
Copy link
Contributor

I put a bunch of comments on the PR you just closed: #3320

@lo-jason
Copy link
Contributor Author

I put a bunch of comments on the PR you just closed: #3320

@jkwatson thank you for the comments. I have made the changes suggested.
Sorry about the duplicate PR i had the wrong submitter email and had to generate a new PR.

ghost
ghost reviewed Jun 17, 2021
View reviewed changes
...ions/resources/src/main/java/io/opentelemetry/sdk/extension/resources/ContainerResource.java Show resolved Hide resolved
...ions/resources/src/main/java/io/opentelemetry/sdk/extension/resources/ContainerResource.java Outdated
*/
@Nullable
@SuppressWarnings("DefaultCharset")
public String extractContainerId() {
Copy link

Choose a reason for hiding this comment

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

Can be made package-protected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

...ions/resources/src/main/java/io/opentelemetry/sdk/extension/resources/ContainerResource.java Outdated Show resolved Hide resolved
@jkwatson
Copy link
Contributor

The fullConfigTest has started failing on CI in more than one PR now. I'm investigating, but I don't know what's going on yet.

@jkwatson
Copy link
Contributor

@lo-jason sorry for the trouble, but you'll need to rebase against the main branch to get the fixes for the failing tests.

Thanks!

@pavankrish123 pavankrish123 mentioned this pull request Jun 21, 2021
Closed
jkwatson
jkwatson approved these changes Jun 22, 2021
View reviewed changes
Copy link
Contributor

@jkwatson jkwatson left a comment

Choose a reason for hiding this comment

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

Thanks!

anuraaga
anuraaga reviewed Jun 25, 2021
View reviewed changes
...ions/resources/src/main/java/io/opentelemetry/sdk/extension/resources/ContainerResource.java Outdated Show resolved Hide resolved
...ions/resources/src/main/java/io/opentelemetry/sdk/extension/resources/ContainerResource.java Outdated Show resolved Hide resolved
...src/main/resources/META-INF/services/io.opentelemetry.sdk.autoconfigure.spi.ResourceProvider Outdated Show resolved Hide resolved
.../resources/src/test/java/io/opentelemetry/sdk/extension/resources/ContainerResourceTest.java Outdated Show resolved Hide resolved
.../resources/src/test/java/io/opentelemetry/sdk/extension/resources/ContainerResourceTest.java Outdated Show resolved Hide resolved
.../resources/src/test/java/io/opentelemetry/sdk/extension/resources/ContainerResourceTest.java Outdated Show resolved Hide resolved
.../resources/src/test/java/io/opentelemetry/sdk/extension/resources/ContainerResourceTest.java Outdated Show resolved Hide resolved
.../resources/src/test/java/io/opentelemetry/sdk/extension/resources/ContainerResourceTest.java Show resolved Hide resolved
...ions/resources/src/main/java/io/opentelemetry/sdk/extension/resources/ContainerResource.java Outdated Show resolved Hide resolved
...ions/resources/src/main/java/io/opentelemetry/sdk/extension/resources/ContainerResource.java Outdated Show resolved Hide resolved
@lo-jason lo-jason closed this Jun 30, 2021
@lo-jason lo-jason deleted the container branch June 30, 2021 02:07
@lo-jason lo-jason restored the container branch June 30, 2021 02:07
@lo-jason lo-jason reopened this Jun 30, 2021
anuraaga
anuraaga reviewed Jun 30, 2021
View reviewed changes
...ions/resources/src/main/java/io/opentelemetry/sdk/extension/resources/ContainerResource.java Outdated Show resolved Hide resolved
...ions/resources/src/main/java/io/opentelemetry/sdk/extension/resources/ContainerResource.java Outdated Show resolved Hide resolved
...ions/resources/src/main/java/io/opentelemetry/sdk/extension/resources/ContainerResource.java Outdated Show resolved Hide resolved
...ions/resources/src/main/java/io/opentelemetry/sdk/extension/resources/ContainerResource.java Outdated Show resolved Hide resolved
...ions/resources/src/main/java/io/opentelemetry/sdk/extension/resources/ContainerResource.java Outdated Show resolved Hide resolved
...ions/resources/src/main/java/io/opentelemetry/sdk/extension/resources/ContainerResource.java Outdated Show resolved Hide resolved
.../resources/src/test/java/io/opentelemetry/sdk/extension/resources/ContainerResourceTest.java Outdated Show resolved Hide resolved
@jkwatson
Copy link
Contributor

@lo-jason looks like this needs a rebase in order to pick up the latest changes to the build. Thanks!

anuraaga
anuraaga approved these changes Jul 20, 2021
View reviewed changes
Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Just tiny nit, thanks!

...ions/resources/src/main/java/io/opentelemetry/sdk/extension/resources/ContainerResource.java Show resolved Hide resolved
@jkwatson jkwatson merged commit 2cb461d into open-telemetry:main Jul 21, 2021
@lo-jason lo-jason deleted the container branch November 16, 2021 02:55
@XSAM XSAM mentioned this pull request Nov 26, 2021
Merged
@swetharavichandrancisco swetharavichandrancisco mentioned this pull request Dec 16, 2021
Closed
This was referenced Dec 19, 2021
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkwatson jkwatson jkwatson approved these changes

@anuraaga anuraaga anuraaga approved these changes

@arminru arminru Awaiting requested review from arminru

@bogdandrutu bogdandrutu Awaiting requested review from bogdandrutu

@carlosalberto carlosalberto Awaiting requested review from carlosalberto

@Oberon00 Oberon00 Awaiting requested review from Oberon00

@pavolloffay pavolloffay Awaiting requested review from pavolloffay

@thisthat thisthat Awaiting requested review from thisthat

@tylerbenson tylerbenson Awaiting requested review from tylerbenson

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lo-jason @jkwatson @anuraaga