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

add environment attribute: Deployment Environment #606

Merged
merged 2 commits into from
Aug 21, 2020

Conversation

ecourreges-orange
Copy link
Contributor

@ecourreges-orange ecourreges-orange commented May 14, 2020

Following @arminru 's advice:

I can't find the Deployement Environment concept in resource semantic conventions so I added it.

I called the attribute environment without any type.

We might have a bit of a naming conflict for the sections "Deployment Service" and "Environment -> Deployment Environment"

Thanks for your comments.

Fixes #593


| Attribute | Description | Example | Required? |
|---|---|---|---|
| environment | Name of the [deployment environment](https://en.wikipedia.org/wiki/Deployment_environment) | `staging` , `production` | Yes |
Copy link
Member

Choose a reason for hiding this comment

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

I think a less overloaded term for this might be "stage". Environment already means too many things.

Copy link
Member

Choose a reason for hiding this comment

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

stage sounds good! Environment could really be anything from the OS, cloud provider to the env variables and much more.

Copy link
Member

Choose a reason for hiding this comment

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

What about just deployment_environment? You might also have different production environments like "free", "vip", so I'd say using environments for staged deployments is just one use case.

Copy link
Member

Choose a reason for hiding this comment

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

Or, if my understanding of the term is wrong, the linked Wikipedia entry suggests the synonym tier.

Copy link
Member

Choose a reason for hiding this comment

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

The term tier is already preoccupied by multi-tier architectures ([frontend]<->[application/businesslogic]<->[database]) but I agree that we should consider the possibility of multiple deployments in the same stage of maturity.

Copy link
Member

Choose a reason for hiding this comment

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

deployment_environment sounds good to me. I agree that stage is just one use case for multi-environment.

Copy link
Contributor Author

@ecourreges-orange ecourreges-orange May 14, 2020

Choose a reason for hiding this comment

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

What about a deployment category with deployment.environment?
I am also looking to add something like deployment.datacenter or hosting.datacenter
For those of us not on a cloud deployment, calling my datacenter cloud.region is slightly abusive

@arminru
Copy link
Member

arminru commented May 14, 2020

I called the attribute environment without any type.

I don't think it's beneficial to force users to map their internal names to a rigid set of pre-defined values as this would just confuse those looking at the monitoring system, so allowing arbitrary strings should indeed be just fine here.

@ecourreges-orange
Copy link
Contributor Author

I called the attribute environment without any type.

I don't think it's beneficial to force users to map their internal names to a rigid set of pre-defined values as this would just confuse those looking at the monitoring system, so allowing arbitrary strings should indeed be just fine here.

Sorry, I meant no type as in cloud or k8s or other types displayed on the same page, but if I would have given it a type, it would have been deployment with an attribute such as deployment.environment or deployment.tier

@arminru
Copy link
Member

arminru commented May 14, 2020

I called the attribute environment without any type.

I don't think it's beneficial to force users to map their internal names to a rigid set of pre-defined values as this would just confuse those looking at the monitoring system, so allowing arbitrary strings should indeed be just fine here.

Sorry, I meant no type as in cloud or k8s or other types displayed on the same page, but if I would have given it a type, it would have been deployment with an attribute such as deployment.environment or deployment.tier

Ah now I get you :-)
It could make sense to introduce deployment as its own type. Would there be any additional deployment-related attributes you could think of that would be of value?

@dyladan
Copy link
Member

dyladan commented May 14, 2020

I called the attribute environment without any type.

I don't think it's beneficial to force users to map their internal names to a rigid set of pre-defined values as this would just confuse those looking at the monitoring system, so allowing arbitrary strings should indeed be just fine here.

Sorry, I meant no type as in cloud or k8s or other types displayed on the same page, but if I would have given it a type, it would have been deployment with an attribute such as deployment.environment or deployment.tier

Ah now I get you :-)
It could make sense to introduce deployment as its own type. Would there be any additional deployment-related attributes you could think of that would be of value?

Might I suggest.... deployment.stage? 😉

@arminru arminru added area:semantic-conventions Related to semantic conventions spec:resource Related to the specification/resource directory labels May 14, 2020
@reyang reyang added the release:after-ga Not required before GA release, and not going to work on before GA label Jul 6, 2020
@bogdandrutu bogdandrutu closed this Aug 4, 2020
@bogdandrutu bogdandrutu reopened this Aug 4, 2020
@bogdandrutu
Copy link
Member

The CLA not does not want to trigger the check can you reopen this PR?

@ecourreges-orange
Copy link
Contributor Author

EasyCLA is successful now.
I decided to merge Deployment Service into Environment, so that deployment service and deployment.environment are close to each other, avoiding any possible confusion for a first time reader.

@Oberon00
Copy link
Member

Oberon00 commented Aug 10, 2020

I think this can be merged @open-telemetry/technical-committee EDIT: Sorry, I see all approvals are only from Dynatrace so not ready to merge after all.

@tigrannajaryan if you are OK with this PR now, you could approve.

@carlosalberto
Copy link
Contributor

Looks good to go! Let's wait 1 day and LGTM.

Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

LGTM

Merge "Deployment Service / Kubernetes" section into "Environment" section for consistency
Create deployment_environment.md for new deployment.environment attribute

Signed-off-by: CI-Bot for Emmanuel Courreges <emmanuel.courreges@orange.com>

- [Operating System](./os.md)
- [Cloud](./cloud.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.

s/Environment/ProductionEnvironment?

Or maybe "Environment Stage"?

"Environment" is a bit too generic I feel like.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please address the feedback here (either way) @ecourreges-orange ;)

@carlosalberto
Copy link
Contributor

@ecourreges-orange Sorry for the delayed feedback - once you address a latest detail regarding the (too generic) usage of "Envirionment", we are totally ready to merge.

@bogdandrutu
Copy link
Member

@ecourreges-orange ping :)

@carlosalberto
Copy link
Contributor

Merging, will follow up with @bogdandrutu's suggestion.

@carlosalberto carlosalberto merged commit 376e5f4 into open-telemetry:master Aug 21, 2020
@arminru
Copy link
Member

arminru commented Aug 21, 2020

Merging, will follow up with @bogdandrutu's suggestion.

@carlosalberto Thanks! Please also add the new attribute to the changelog while you're at it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:semantic-conventions Related to semantic conventions release:after-ga Not required before GA release, and not going to work on before GA spec:resource Related to the specification/resource directory
Projects
None yet
9 participants