fix: add missing platform and cloud provider values #1857
Conversation
anupamdalmia10
requested review from
jmacd,
MrAlias,
Aneurysm9,
evantorrie,
XSAM,
dashpole,
paivagustavo,
MadVikingGod and
pellared
as code owners
Add PR number
|
My bad, i'll update the failing test cases |
…itor/opentelemetry-go-contrib into add-cloud-platform-to-detectors
|
Fixed the failing test cases |
|
Looks like test cases from different module are failing... |
Tests in |
working on it |
@Aneurysm9 , updated the test cases in the concerned package |
Codecov Report
@@ Coverage Diff @@
## main #1857 +/- ##
=======================================
- Coverage 72.1% 69.6% -2.6%
=======================================
Files 124 127 +3
Lines 5330 5396 +66
=======================================
- Hits 3848 3757 -91
- Misses 1370 1501 +131
- Partials 112 138 +26
|
|
This particular check "codecov/patch" is failing because there is a change in both gce and gke detectors and the test cases/files for those do not exist as of now. Shall those be added as a part of this PR or are the changes good to be merged? |
It would be appreciated if you wanted to add coverage for these here. |
Sure @MrAlias , I'll work on the test cases and update the PR |
| @@ -18,6 +18,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm | |||
| These attributes are detected automatically, but it is also now possible to provide a custom function to set attributes using `WithAttributeSetter`. (#1582) | |||
| - Add resource detector for GCP cloud function. (#1584) | |||
| - Add OpenTracing baggage extraction to the OpenTracing propagator in `go.opentelemetry.io/contrib/propagators/ot`. (#1880) | |||
| - Add missing `cloud.platform` value to AWS Lambda, GCP Compute Engine, GCP Cloud Run, GCP GKE resource detectors and add `cloud.provider` to GCP GKE resource detector. (#1857) | |||
There was a problem hiding this comment.
Please move this to the Unreleased section.
| type GCE struct { | ||
| mc metadataClient | ||
| onGCE func() bool | ||
| } | ||
|
|
||
| var NewGCE = func() *GCE { | ||
| return &GCE{ | ||
| mc: metadata.NewClient(nil), | ||
| onGCE: metadata.OnGCE, | ||
| } | ||
| } | ||
|
|
||
| // compile time assertion that GCE implements the resource.Detector interface. | ||
| var _ resource.Detector = (*GCE)(nil) | ||
|
|
||
| // Detect detects associated resources when running on GCE hosts. | ||
| func (gce *GCE) Detect(ctx context.Context) (*resource.Resource, error) { | ||
| if !metadata.OnGCE() { | ||
| if !gce.onGCE() { |
There was a problem hiding this comment.
This is an exported type in a stable package. It needs to be considered what will happen for an existing user that is declaring a new variable of this type already (i.e. GCE{}) and they upgrade: the onGCE field will be nil and this will panic.
This also exports a new variable (I'm not sure why is this isn't func declaration?). I would suggest we try and preserve the existing initialization behavior of the GCE type as best we can to support existing uses and not include this new var. Also, if we ultimately want to add this, it should not be done in a PR dedicated to the addition (not in a PR adding semconv values).
I expect this was done to accommodate mocking while testing. Instead of this approach, I would suggest abstract the metadata package functions used to be unexported package variables that a test could overwrite.
| } | ||
|
|
||
| // NewCloudRun creates a CloudRun detector. | ||
| func NewGKE() *GKE { |
Add missing cloud.platform value to AWS Lambda, GCP Compute Engine, GCP Cloud Run, GCP GKE resource detectors and add cloud.provider to GCP GKE resource detector.