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

Proposal: Add a version semantic attribute #38

Merged
merged 12 commits into from
Oct 8, 2019

Conversation

austinlparker
Copy link
Member

This proposes a resource (or sub-resource) that can identify the version of another resource.

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.

The Resource currently allows to specify arbitrary key/value pairs. Is there a need to have Version as a structured element rather than as a key/value pair with a pre-defined key value such as "version"? I am curious as to how version is different than other defining attributes of a resource.

```
{
"version": {
"type": "semver",
Copy link
Member

Choose a reason for hiding this comment

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

Why not "semver": "1.0.0"?

Copy link
Contributor

@z1c0 z1c0 Sep 19, 2019

Choose a reason for hiding this comment

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

Is this meant in addition to existing resource objects?
e.g.

{
	"type": "container",
	"labels": {
           ....
	},
       "version" : {
       ....
       }
}

(example from OpenCensus: https://github.com/census-instrumentation/opencensus-specs/blob/master/resource/Resource.md#exporter-translation)

@austinlparker
Copy link
Member Author

The biggest rationale I had for making the Version a structured element was to support the notion that there can be multiple types of version strings that may have different meaning to a consumer. For example, it's quite frequent to tag releases in cloud native systems with their git SHA rather than a semver string.

That said, I think it'd also work to simply say version is a key on each resource and any string can be set as the value, at which point it'd be incumbent on a backend to figure out ways to differentiate between them.

@toumorokoshi
Copy link
Member

Regardless of the medium, I think version will play a big factor, especially in metrics.
Working through the problem myself I was thinking Resource would be a great place to put it, considering it's a static context collection that's available to reference.

I feel like if it's going to be defined in the spec, we should have some clear uses cases. What will this RFC enable us to do?

@yurishkuro
Copy link
Member

@austinlparker there's a simpler way to achieve that with just the version attribute of the resource - by using a naming scheme, e.g. version=semver:1.2.3 or version=git:8ae73a.

I am curious if the type of version actually matters, i.e. what exactly would one do differently knowing that the version is semver vs. git sha?

Oberon00 pushed a commit to dynatrace-oss-contrib/oteps that referenced this pull request Sep 16, 2019
* Add Tracer operations specs.

Fixes open-telemetry#38.

* recordSpanData should be async.

* Fix typos.

* Fix review comments.

* Fix another typo.

Co-Authored-By: Mayur Kale <mayurkale@google.com>
@z1c0 z1c0 mentioned this pull request Sep 20, 2019
@bogdandrutu
Copy link
Member

bogdandrutu commented Sep 21, 2019

For TC members please vote:
I am proposing to "rejected" this otep based on the suggestions from #38 (comment).

@bogdandrutu @yurishkuro @carlosalberto @SergeyKanzhelev

@austinlparker
Copy link
Member Author

I'd rather modify it per Yuri's comments, they make sense.

@bogdandrutu
Copy link
Member

@austinlparker that will become a simple semantic convention for the version resource label or Span attribute

@austinlparker
Copy link
Member Author

Yes, but wouldn't changes to semantic conventions also go through the RFC process?

@bogdandrutu
Copy link
Member

I would say this is a very simple change that I would be happy to just review in the specs repo.

My 2c: Let's try to keep a balance between what is required in RFC vs a small change/improvement in specs.

@austinlparker
Copy link
Member Author

I originally brought this up in the spec repo and it was suggested as an RFC (see open-telemetry/opentelemetry-specification#235). If y'all want it to go back there fine, but other people are making refs to this.

@bogdandrutu
Copy link
Member

Disclaimer: I am pretty new in the RFC model, but my understanding is that RFC are required when some important and complicated changes are proposed. Not trying to say that this is not important, but it becomes a very simple change if we follow Yuri's comment.

@bogdandrutu
Copy link
Member

In the current form I am happy to approve this, but I feel that you did a lot more work to get some minor change in the semantic conventions. I would like to hear others opinion, but don't want to block your work because I think it is good work.

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.

I recommend changing this to APPROVED status, to reduce friction.

@carlosalberto
Copy link
Contributor

Looks fine to me either way - probably we should do as @yurishkuro says and approve it to reduce friction, and try to have this kind of relatively straightforward changes as Specification Issues instead (for the next times, that is).

Probably we could also add a small note on that, as part of the RFC process docs?

austinlparker and others added 3 commits September 23, 2019 12:07
Co-Authored-By: Yuri Shkuro <yurishkuro@users.noreply.github.com>
@austinlparker
Copy link
Member Author

assigned 0038 and marked as approved per comments

@bogdandrutu
Copy link
Member

Can we please get this reviewed by one more approver?

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

@bogdandrutu bogdandrutu changed the title Proposal: Add a version resource Proposal: Add a version semantic convention Oct 8, 2019
@bogdandrutu bogdandrutu changed the title Proposal: Add a version semantic convention Proposal: Add a version semantic attribute Oct 8, 2019
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.

7 participants