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

Added google cloud function resource detector #1584

Merged
merged 20 commits into from Mar 4, 2022

Conversation

patil-kshitij
Copy link
Contributor

Added google cloud-function resource detector
Added following attributes -

  • projectID
  • region
  • functionName
  • cloud.provider
  • cloud.platform

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 13, 2022

CLA Signed

The committers are authorized under a signed CLA.

@dashpole dashpole self-assigned this Jan 13, 2022
@codecov
Copy link

codecov bot commented Jan 13, 2022

Codecov Report

Merging #1584 (5fd26ff) into main (9e8d62b) will increase coverage by 0.1%.
The diff coverage is 100.0%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #1584     +/-   ##
=======================================
+ Coverage   70.3%   70.5%   +0.1%     
=======================================
  Files        128     129      +1     
  Lines       5537    5564     +27     
=======================================
+ Hits        3898    3925     +27     
  Misses      1500    1500             
  Partials     139     139             
Impacted Files Coverage Δ
detectors/gcp/cloud-function.go 100.0% <100.0%> (ø)

detectors/gcp/cloud-function.go Outdated Show resolved Hide resolved
detectors/gcp/cloud-function.go Outdated Show resolved Hide resolved
return nil,nil when not on cloud-function
Copy link
Contributor

@dashpole dashpole left a comment

Choose a reason for hiding this comment

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

Can you add unit tests? They should be very similar to the cloud run unit tests: https://github.com/open-telemetry/opentelemetry-go-contrib/blob/main/detectors/gcp/cloud-run_test.go#L82

detectors/gcp/cloud-function.go Outdated Show resolved Hide resolved
@patil-kshitij
Copy link
Contributor Author

Can you add unit tests? They should be very similar to the cloud run unit tests: https://github.com/open-telemetry/opentelemetry-go-contrib/blob/main/detectors/gcp/cloud-run_test.go#L82

Okay, will add unit test cases.

@patil-kshitij
Copy link
Contributor Author

Can you add unit tests? They should be very similar to the cloud run unit tests: https://github.com/open-telemetry/opentelemetry-go-contrib/blob/main/detectors/gcp/cloud-run_test.go#L82

Hi @dashpole ,
Have added unit test cases,
Can you please review ?

@patil-kshitij
Copy link
Contributor Author

Hi @dashpole ,
Can you please approve for workflows ?

@patil-kshitij
Copy link
Contributor Author

Hi @Aneurysm9
Can you please review ?

detectors/gcp/cloud-function.go Outdated Show resolved Hide resolved
detectors/gcp/cloud-function_test.go Show resolved Hide resolved
Copy link
Member

@Aneurysm9 Aneurysm9 left a comment

Choose a reason for hiding this comment

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

Please add a CHANGELOG.md entry for this addition.

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.

The initialization of the CloudFunction type needs to be addressed before we can merge this.

detectors/gcp/cloud-function.go Outdated Show resolved Hide resolved
detectors/gcp/cloud-function.go Outdated Show resolved Hide resolved
detectors/gcp/cloud-function.go Outdated Show resolved Hide resolved
detectors/gcp/cloud-function.go Outdated Show resolved Hide resolved
detectors/gcp/cloud-function.go Outdated Show resolved Hide resolved
detectors/gcp/cloud-function.go Outdated Show resolved Hide resolved
detectors/gcp/cloud-function_test.go Outdated Show resolved Hide resolved
detectors/gcp/cloud-function_test.go Show resolved Hide resolved
@MrAlias
Copy link
Contributor

MrAlias commented Feb 17, 2022

@patil-kshitij are you still able to work on this and apply the feedback?

@patil-kshitij
Copy link
Contributor Author

@patil-kshitij are you still able to work on this and apply the feedback?

Sure, @MrAlias , will work on the feedback and apply those changes

@patil-kshitij
Copy link
Contributor Author

Please add a CHANGELOG.md entry for this addition.

Hi @Aneurysm9 , have added changelog, can you please review ?

CHANGELOG.md Outdated Show resolved Hide resolved
@MrAlias MrAlias merged commit 6ee1fdd into open-telemetry:main Mar 4, 2022
Copy link
Member

@hanyuancheung hanyuancheung left a comment

Choose a reason for hiding this comment

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

lgtm, a little suggestion!

@@ -13,6 +13,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm

- DynamoDB spans created with the `go.opentelemetry.io/contrib/instrumentation/github.com/aws/aws-sdk-go-v2/otelaws` package will now have the appropriate database attributes added for the operation being performed.
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.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Add resource detector for GCP cloud function.
- Add resource detector for GCP cloud function. (#1584)

gcpFunctionNameKey = "K_SERVICE"
)

// NewCloudFunction will return a GCP Cloud Function resource detector.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// NewCloudFunction will return a GCP Cloud Function resource detector.
// NewCloudFunction returns a GCP Cloud Function resource detector.

@MrAlias MrAlias mentioned this pull request Mar 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants