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

[processor/resourcedetection] Add new GCP resource attributes for GCE instance name/hostname #24598

Merged

Conversation

damemi
Copy link
Contributor

@damemi damemi commented Jul 26, 2023

Description:
This uses new detector functions from GoogleCloudPlatform/opentelemetry-operations-go#669 to update the GCP resource detector to add new GCE-specific attributes for hostname and instance name. These attributes are meant to more clearly express their intent in specific context to GCE. See https://github.com/open-telemetry/semantic-conventions/blob/main/docs/resource/cloud-provider/gcp/gce.md

Link to tracking Issue: GoogleCloudPlatform/opentelemetry-operations-go#511

Testing: unit tests updated, plus downstream unit and integration tests

Documentation: matches semantic conventions

@damemi damemi requested a review from a team July 26, 2023 18:48
@github-actions github-actions bot added the processor/resourcedetection Resource detection processor label Jul 26, 2023
@damemi damemi force-pushed the gcp-gce-hostname-resourcedetection branch from d3191a4 to dd71779 Compare July 26, 2023 18:49
@damemi
Copy link
Contributor Author

damemi commented Jul 26, 2023

Note: this will conflict with #23681 when one of them merges. Also, like that one, these will eventually be updated to use semantic convention library constants instead of string literals for the attributes

@damemi damemi force-pushed the gcp-gce-hostname-resourcedetection branch from dd71779 to 7922981 Compare July 26, 2023 19:08
Comment on lines 54 to 69
description: The name of the GCE instance.
type: string
enabled: true
gcp.gce.instance.hostname:
description: The hostname of the GCE instance.
type: string
enabled: true
Copy link
Member

Choose a reason for hiding this comment

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

As I mentioned in #23681 (comment), new resource attributes should be added as optional initially. Otherwise, it's considered a breaking change. If we want to make them enabled by default, we will need to apply a warning to notify users in advance after they are added as optional. We'll get an option to add such warnings in metadata.yaml once #19174 is resolved. I've done some prerequisite work for that, and will add the warnings option next week.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, got confused because we reverted that with #23681 (comment), but I forgot that's because Cloud Run Jobs were totally new. I updated this to disable the new GCE attributes by default

@damemi damemi force-pushed the gcp-gce-hostname-resourcedetection branch from 7922981 to 8c15e37 Compare July 27, 2023 14:54
* faas.id (instance id)
* faas.name (service name)
* gcp.cloud_run.job.execution ("my-service-ajg89")
* gcp.cloud_run.job.task_index ("0")
Copy link
Member

@crobert-1 crobert-1 Jul 27, 2023

Choose a reason for hiding this comment

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

Pardon my ignorance here, but should the new resource attributes be included in this list, or are they irrelevant here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The attributes for cloud run jobs should have been added in #23681, and they're enabled by default, so I do think so.

The optional gcp.gce.instance.hostname and gcp.gce.instance.name (this PR, disabled by default) I wasn't sure about, so I labeled them as optional in the readme.

@songy23 songy23 added the ready to merge Code review completed; ready to merge by maintainers label Aug 3, 2023
@mx-psi
Copy link
Member

mx-psi commented Aug 4, 2023

/easycla

@mx-psi mx-psi requested a review from dmitryax August 4, 2023 14:35
@TylerHelmuth TylerHelmuth merged commit 1c47e4f into open-telemetry:main Aug 4, 2023
88 checks passed
@github-actions github-actions bot added this to the next release milestone Aug 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
processor/resourcedetection Resource detection processor ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants