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 cloud.infrastructure_service attribute to resource spec #1112

Merged
merged 15 commits into from
Jan 7, 2021

Conversation

willarmiros
Copy link
Contributor

@willarmiros willarmiros commented Oct 19, 2020

Resolves #1151. Resolves #907.

Changes

This PR adds a new attribute to cloud.md: cloud.infrastructure_service. The new attribute is a single-value descriptor of the type of compute infrastructure, with several suggested values for the most popular compute platforms from the currently supported cloud providers. This takes out the guess work for users who may be interested in seeing what platform their telemetry is originating from. So, for example, instead of having to notice cloud.provider == aws and faas.id is defined in order to know this trace is coming from AWS Lambda, it will just be defined concretely here.

I'd appreciate someone from GCP and Azure giving signoff on the names and descriptions for the services from their platforms :)

Split off from #1099 on the recommendation of @anuraaga.

willarmiros and others added 2 commits October 19, 2020 18:23
Co-authored-by: Anuraag Agrawal <anuraaga@gmail.com>
@@ -37,3 +37,55 @@ groups:
note: >
In AWS, this is called availability-zone.
examples: ['us-central1-a']
- id: infrastructure.service
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry my PR suggestion was in the wrong place - should have renamed here and then regenerated the md

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh duh, will fix

@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Oct 27, 2020
@anuraaga
Copy link
Contributor

@tigrannajaryan @open-telemetry/specs-approvers do you mind taking a look at this? Thanks!

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

Not a fan of enumerating all proprietary names of services like that. What about names used in IBM Cloud, Oracle Cloud, Alibaba Cloud, etc.... ? Where does it end?

@anuraaga
Copy link
Contributor

@yurishkuro Ends when there isn't interest in certain service names and resumes when it returns? :)

It seems to companion cloud.provider well by providing some more information about the cloud service, which we've found to be pretty useful. If not a generic attribute, we'd encourage aws.infrastructure_service to provide information about AWS services but it feels generally useful.

@github-actions github-actions bot removed the Stale label Oct 28, 2020
@tigrannajaryan
Copy link
Member

If not a generic attribute, we'd encourage aws.infrastructure_service to provide information about AWS services but it feels generally useful.

I think this is the call we need to make. Do we record provider-specific data as values of provider-independent attribute names (e.g. cloud.infrastructure_service=EC2 like this PR suggests) or each provider uses their own namespace and then can create provider-specific attributes within the namespace (e.g. cloud.aws.service=EC2 or cloud.azure.service=VM)?

I think this warrants a separate discussion since each approach has its cons and pros. @willarmiros @anuraaga would you mind creating an issue to discuss this first and then this PR will follow whatever decision we make.

@willarmiros
Copy link
Contributor Author

@tigrannajaryan @anuraaga @yurishkuro I've made #1151 to continue discussion.

@github-actions
Copy link

github-actions bot commented Nov 5, 2020

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Nov 5, 2020
@willarmiros willarmiros changed the title added cloud.infrastructure.service attribute to resource spec added cloud.infrastructure_service attribute to resource spec Nov 5, 2020
@willarmiros
Copy link
Contributor Author

@tigrannajaryan @yurishkuro @anuraaga any chance you and any other stakeholders could weigh in on #1151 to make a decision here?

@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Nov 13, 2020
Copy link
Member

@arminru arminru left a comment

Choose a reason for hiding this comment

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

I think it's a sound approach. Stakeholders are free to add their services to the enum as they see fit. Instrumentations providing this attribute only need to know about subset of values they apply to and back ends only have limited platform support already (often with generic fallbacks for unknown values) and thus only need to handle the platforms they claim to support. Maintaining the enum centrally here can be a bit cumbersome but this shouldn't be a big deal IMHO and ensures different instrumentations in different frameworks and languages stay aligned.

@yurishkuro could you please take another look or otherwise comment on the alternative proposed in #1151? Thanks!

semantic_conventions/resource/cloud.yaml Outdated Show resolved Hide resolved
@arminru arminru requested a review from yurishkuro November 16, 2020 17:10
Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
semantic_conventions/resource/cloud.yaml Outdated Show resolved Hide resolved
@@ -37,3 +37,55 @@ groups:
note: >
In AWS, this is called availability-zone.
examples: ['us-central1-a']
- id: infrastructure_service
Copy link
Member

Choose a reason for hiding this comment

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

I agree. However, given that "infrastructure" could be interpreted in a broader sense, it should be fine, and the enumeration of well-known values will also make it clearer.

@arminru arminru requested review from anuraaga and a team December 9, 2020 16:01
willarmiros and others added 2 commits December 9, 2020 10:06
Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
@yurishkuro yurishkuro removed their request for review December 9, 2020 18:32
@arminru arminru removed the Stale label Dec 10, 2020
@willarmiros
Copy link
Contributor Author

@arminru any chance this could be merged now? :)

@arminru arminru requested a review from a team December 17, 2020 10:22
@arminru
Copy link
Member

arminru commented Dec 17, 2020

@willarmiros Sorry for the long delay on your PR.
Usually we try to get approvals from at least two companies that are different from the PR author's, so I hope some of the other @open-telemetry/specs-approvers will have a look as well.
Let's give them until the end of the week and otherwise merge it, the minimum requirements are fulfilled.

@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Dec 25, 2020
@willarmiros
Copy link
Contributor Author

Stale bot clearly doesn't respect the holiday break :)

@github-actions github-actions bot removed the Stale label Dec 29, 2020
@github-actions
Copy link

github-actions bot commented Jan 5, 2021

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Jan 5, 2021
@anuraaga
Copy link
Contributor

anuraaga commented Jan 5, 2021

@arminru As the time has passed think we can go ahead and merge this?

@github-actions github-actions bot removed the Stale label Jan 6, 2021
@arminru arminru merged commit 99b856f into open-telemetry:master Jan 7, 2021
@willarmiros willarmiros deleted the infra-service branch January 13, 2021 22:56
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
…elemetry#1112)

Co-authored-by: Anuraag Agrawal <anuraaga@gmail.com>
Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants