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

GCE Resource Detector #132

Merged
merged 16 commits into from
Jul 24, 2020
Merged

GCE Resource Detector #132

merged 16 commits into from
Jul 24, 2020

Conversation

YANYZP
Copy link
Contributor

@YANYZP YANYZP commented Jul 10, 2020

This is an vendor implementation of detector.
Refer to open-telemetry/opentelemetry-go#924

@YANYZP
Copy link
Contributor Author

YANYZP commented Jul 13, 2020

Tested it on GCE environment and got expected results.
Ready for final review.

lizthegrey
lizthegrey suggested changes Jul 13, 2020
View reviewed changes
Copy link
Member

@lizthegrey lizthegrey left a comment

Choose a reason for hiding this comment

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

Thanks for contributing! Some initial things we'll need to address before merging:

detect/gcp/gcp.go Outdated Show resolved Hide resolved
detect/gcp/gcp.go Outdated Show resolved Hide resolved
detect/gcp/gcp.go Outdated Show resolved Hide resolved
detect/gcp/gcp.go Outdated Show resolved Hide resolved
lizthegrey
lizthegrey suggested changes Jul 14, 2020
View reviewed changes
detect/gcp/gcp.go Outdated Show resolved Hide resolved
detect/gcp/gcp.go Outdated Show resolved Hide resolved
detect/gcp/gcp.go Outdated Show resolved Hide resolved
YANYZP
YANYZP commented Jul 14, 2020
View reviewed changes
Copy link
Contributor Author

@YANYZP YANYZP left a comment

Choose a reason for hiding this comment

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

Updates with aggregating multiple errors

detect/gcp/gcp.go Outdated Show resolved Hide resolved
detect/gcp/gcp.go Outdated Show resolved Hide resolved
@Aneurysm9
Copy link
Member

Should the go.opentelemetry.io/contrib/detect/gcp package have its own go.mod? I think that would avoid pulling in its dependencies to all of the other instrumentation modules that depend on the go.opentelemetry.io/contrib mod.

@YANYZP
Copy link
Contributor Author

YANYZP commented Jul 15, 2020

Update hostname function and separate gcp into a module

@YANYZP
Copy link
Contributor Author

YANYZP commented Jul 20, 2020

Named this detector GCE detector and plan to implement GKE detector under the same package gcp.

MrAlias
MrAlias reviewed Jul 20, 2020
View reviewed changes
detect/gcp/go.mod Outdated Show resolved Hide resolved
go.sum Outdated Show resolved Hide resolved
detect/gcp/gce.go Outdated Show resolved Hide resolved
detect/gcp/gce.go Outdated Show resolved Hide resolved
detect/gcp/gce.go Outdated Show resolved Hide resolved
@YANYZP YANYZP changed the title Gcp detector GCP detector Jul 20, 2020
@YANYZP
Copy link
Contributor Author

YANYZP commented Jul 20, 2020

Optimized error handling part and cleaned go.sum

MrAlias
MrAlias reviewed Jul 20, 2020
View reviewed changes
detectors/gcp/gce.go Outdated Show resolved Hide resolved
MrAlias
MrAlias reviewed Jul 21, 2020
View reviewed changes
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

Looks good, only remaining issue is in hasProblem. Otherwise, I think this is ready to merge.

@lizthegrey can you take another look?

detectors/gcp/gce.go Outdated Show resolved Hide resolved
detectors/gcp/gce.go Outdated Show resolved Hide resolved
@MrAlias MrAlias requested a review from lizthegrey July 21, 2020 15:18
@MrAlias MrAlias changed the title GCP detector GCE Resource Detector Jul 22, 2020
MrAlias
MrAlias approved these changes Jul 22, 2020
View reviewed changes
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

Can we add this change to the CHANGELOG?

@YANYZP
Copy link
Contributor Author

YANYZP commented Jul 22, 2020

Updated changelog

@YANYZP
Copy link
Contributor Author

YANYZP commented Jul 22, 2020

@Aneurysm9 Do you have some suggestions for this PR? Thank you.

lizthegrey
lizthegrey approved these changes Jul 24, 2020
View reviewed changes
detectors/gcp/go.mod Show resolved Hide resolved
@YANYZP
Copy link
Contributor Author

YANYZP commented Jul 24, 2020

Included detectors/gcp in dependabot.yml

@lizthegrey lizthegrey merged commit 5c2cfc3 into open-telemetry:master Jul 24, 2020
YANYZP pushed a commit to YANYZP/opentelemetry-go-contrib that referenced this pull request Jul 24, 2020
Merge pull request #5 from open-telemetry/master
9010ce5 
GCE Resource Detector (open-telemetry#132)
@MrAlias MrAlias mentioned this pull request Jul 31, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lizthegrey lizthegrey lizthegrey approved these changes

@MrAlias MrAlias MrAlias approved these changes

@Aneurysm9 Aneurysm9 Awaiting requested review from Aneurysm9

@evantorrie evantorrie Awaiting requested review from evantorrie

@jmacd jmacd Awaiting requested review from jmacd

@paivagustavo paivagustavo Awaiting requested review from paivagustavo

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@YANYZP @Aneurysm9 @lizthegrey @MrAlias