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

Remove version convention / semver prefix from versions. #873

Merged
merged 8 commits into from
Aug 26, 2020

Conversation

anuraaga
Copy link
Contributor

Fixes #802

Changes

Removes the convention of prefixing version numbers with things like semver:, git:. This comes from the idea that the version is the version, adding prefixes makes it something that isn't the version, nor a duck. If a user queries their data for a version number, it is very unintuitive to have to prefix queries with semver:, or otherwise every single data store that stores this data needs to natively understand our spec well enough to do this for them which doesn't seem like a worthwhile restriction.

I didn't add a separate attribute for the version type - in the issue, no one championed for it. I can add it if it seems very useful. Need to do it now instead of later since then it needs to be foo.version.id + foo.version.type, we can't have foo.version + foo.version.type based on our conventions for attribute key namespaces.

@anuraaga anuraaga requested a review from a team as a code owner August 25, 2020 07:29
@anuraaga anuraaga requested review from a team August 25, 2020 07:29
@anuraaga anuraaga requested a review from a team as a code owner August 25, 2020 07:29
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.

Changelog please! SIGs should be advised to remove any existing version prefixes added by their SDK and instrumentations.

specification/resource/semantic_conventions/README.md Outdated Show resolved Hide resolved
Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
Copy link
Contributor Author

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Oops, thanks!

@MrAlias
Copy link
Contributor

MrAlias commented Aug 25, 2020

The downside of removing this prefix is there is an inability programmatically interpret this version. Similar to how the OCI realized that the original docker hashing scheme was not adequate to fully describe the image hash by itself (you needed to have prior knowledge of the hashing algorithm and hash size used) they developed a digest concept that is similar.

This approach allowed for the extensibility of the container hash while remaining fully descriptive of the algorithm. I think it points out a valid approach and one that will help the project in the long term if we try to achieve the same goals here with the version.

@Oberon00
Copy link
Member

The downside of removing this prefix is there is an inability programmatically interpret this version.

Agreed, but at the same time, the current spec requires instrumentations to know or guess the format of the version. We could add an raw: or ?: prefix for that but I think this may just be a part that is overengineered. And as mentioned in the PR description, we can always add a companion attribute that tells the version format later.

@tigrannajaryan
Copy link
Member

Need to do it now instead of later since then it needs to be foo.version.id + foo.version.type, we can't have foo.version + foo.version.type based on our conventions for attribute key namespaces.

We can still add foo.version_type if it turns out that we need it in the future after all. So, it is desirable but not mandatory to make the decision right now. IMO, we don't need the version type.

@bogdandrutu
Copy link
Member

I've never seen so many approvers :)

This pull request was closed.
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.

Consider removing semver: prefix from version or moving to another attribute
10 participants