Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally, we should have some directive here to map the old enum values to the new ones.
But according to https://github.com/open-telemetry/oteps/blob/main/text/0152-telemetry-schemas.md, we only have a
rename_attributes
transformation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are actually considering 1.6.0 a broken version, but we should not make this the norm, and go the way you described (hopefully @tigrannajaryan can weight here as to what things need to be done). Sounds about right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was OOO, so don't know the details. Can you please tell why is it broken?
If we consider 1.6.0 a version that was never really released then we can skip it in the schema file. However if 1.6.0 was released, but we don't have a way to describe the changes from 1.6.0 to 1.6.1 it is best to still include 1.6.0 in the schema file, but keep the list of transformations empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're considering 1.6.0 DOA, but the issue was the original semantic conventions were unable to be generated due to violating semcon standards.
We decided to just mark 1.6.0 DOA because there was no adoption (so far) of 1.6.0, and languages were having trouble adopting those semcon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable to me, what do you think @carlosalberto ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tigrannajaryan would this make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I'd go for keeping it the current way, at least for this rather extraordinary situation, as open-telemetry/opentelemetry.io#695 was already merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either way works, no big deal. I don't think anybody has schema files for 1.6.0 in real use.
For the future we can add more automated checks to ensure version list in schema files and version numbers of releases here don't get out of sync.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tigrannajaryan Makes sense!