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

Use Deployment Environment in Resources' README instead of Environment. #851

Merged

Conversation

carlosalberto
Copy link
Contributor

Follow up of #606

CHANGELOG entry is also added.

@carlosalberto carlosalberto requested review from a team August 21, 2020 14:29
Copy link
Member

@Oberon00 Oberon00 left a comment

Choose a reason for hiding this comment

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

It was explicitly discussed that "stage" is not an ideal name, and "deployment environment" is better. E.g. you might have more than one environment per stage. Please do not change the meaning subtly by inventing a new name here. "Tier" was also discussed and discarded for the same reason. See discussion here, were both "stage" and "tier" were considered and rejected #606 (comment)

specification/resource/semantic_conventions/README.md Outdated Show resolved Hide resolved
@carlosalberto carlosalberto added spec:resource Related to the specification/resource directory area:semantic-conventions Related to semantic conventions area:sdk Related to the SDK labels Aug 22, 2020
@carlosalberto
Copy link
Contributor Author

@Oberon00 Updated. Re-review please.

@ecourreges-orange
Copy link
Contributor

Thanks all for the followup and previous merge, I was on vacation.

@carlosalberto
Copy link
Contributor Author

@bogdandrutu You ok with Deployment Environment? ;)

@@ -102,7 +102,7 @@ Attributes defining a running environment (e.g. Operating System, Cloud, Data Ce
- [Operating System](./os.md)
- [Cloud](./cloud.md)
- Deployment:
- [Environment](./deployment_environment.md)
- [Deployment Environment](./deployment_environment.md)
Copy link
Member

Choose a reason for hiding this comment

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

should then the semantic be:
deployment_environment.name?

Copy link
Member

@Oberon00 Oberon00 Aug 24, 2020

Choose a reason for hiding this comment

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

I think the semantic convention is probably fine as it is. But maybe the heading structure should actually be:

  • Cloud
  • Deployment
  • Kubernetes

without putting Kubernetes under Deployment.

Copy link
Member

Choose a reason for hiding this comment

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

That is also an option :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Oberon00 Heading structure of this document (README.md) or the convention itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ping @Oberon00

Copy link
Member

Choose a reason for hiding this comment

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

I was talking about the TOC but the actual structure should most likely match that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm lost at this moment at what you guys expect ;) Please elaborate on what you want to see here regarding:

  1. The TOC.
  2. The actual semantic convention.

I don't have a special preference, and personally think the way it stands now (with this PR) is just fine, but I'm open to suggestions.

Copy link
Member

Choose a reason for hiding this comment

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

I'm also lost 😄

I think the current PR is fine and we can clean this up with #559

@bogdandrutu
Copy link
Member

Please update the title of the PR

@arminru arminru changed the title Use Environment Stage in Resources instead of Environment. Use Deployment Environment in Resources instead of Environment. Aug 24, 2020
@carlosalberto carlosalberto changed the title Use Deployment Environment in Resources instead of Environment. Use Deployment Environment in Resources' README instead of Environment. Aug 24, 2020
@carlosalberto
Copy link
Contributor Author

Merging as this PR has the initial feedback that was left from #606 - remaining bits may be further refined through #559 (as @Oberon00 mentioned).

@carlosalberto carlosalberto merged commit 32d3dc1 into open-telemetry:master Sep 8, 2020
carlosalberto added a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:sdk Related to the SDK area:semantic-conventions Related to semantic conventions spec:resource Related to the specification/resource directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants