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 typed spans working document #14

Merged
merged 2 commits into from May 21, 2019

Conversation

danielkhan
Copy link
Contributor

No description provided.

@danielkhan danielkhan closed this May 17, 2019
@danielkhan danielkhan reopened this May 17, 2019
@danielkhan danielkhan closed this May 17, 2019
@danielkhan danielkhan reopened this May 17, 2019
@bogdandrutu
Copy link
Member

@danielkhan thanks for the PR, you need to sign the CLA.

work_in_progress/typedspans/README.md Outdated Show resolved Hide resolved
work_in_progress/typedspans/README.md Outdated Show resolved Hide resolved
</td>
</tr>
<tr>
<td><strong>db.instance</strong>
Copy link

Choose a reason for hiding this comment

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

These seems contrary to OpenTracing. The mandatory field should be db.type. Why db.instance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe this was a mistake in reading the spec. Looking at https://github.com/opentracing/specification/blob/master/semantic_conventions.md#database-client-calls it seems as if both should be required. Would you agree?

work_in_progress/typedspans/README.md Outdated Show resolved Hide resolved
</td>
</tr>
<tr>
<td><strong>peer.*</strong>
Copy link

Choose a reason for hiding this comment

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

If required, all options should be explicitly defined. Why are any of these required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are required to map them to a concrete instance in a model that might be external to OpenTelemetry.

work_in_progress/typedspans/README.md Outdated Show resolved Hide resolved
work_in_progress/typedspans/README.md Outdated Show resolved Hide resolved
@danielkhan
Copy link
Contributor Author

@danielkhan thanks for the PR, you need to sign the CLA.

Yes we are waiting for our legal team to take care of that.

@danielkhan danielkhan closed this May 20, 2019
@danielkhan danielkhan reopened this May 20, 2019
@danielkhan danielkhan closed this May 20, 2019
@danielkhan danielkhan reopened this May 20, 2019
@tylerbenson
Copy link
Member

Can anyone offer insight as to how we expect this would be implemented? For example, would there be many different sub-classes of Span, each with different sets of fields? Or perhaps a "typed attribute" field inside Span that these would be set on?

@discostu105
Copy link
Member

@tylerbenson IMHO this work should be split in two parts.

1: Just documentation of what attributes exist and which are supposed to be on certain kinds (more like OT "semantic conventions", but with the canonical type notion). Not technically binding, but rather recommendations. Why? It has value both on implementing instrumentation as well as on interpreting data (analytics backend).

2: Typed spans API. This is a different discussion (we should do separately :)). IMHO we should keep generic span classes, with generic span attributes. But we could add typed "Builder" classes that would help a user to set the correct attributes (via dedicated API's for each attribute).

- Added motivation
- Provided a broader scope and less implementation details, like naming or mandatory fields
- Removed tables
@SergeyKanzhelev
Copy link
Member

@danielkhan I think it's a good idea to merge this document in and then start transferring pieces of it to the folder outside the work_in_progress folder. We started by copying open census and open tracing documents into subfolders of this folder and when specific item is described in the document like https://github.com/open-telemetry/opentelemetry-specification/blob/master/work_in_progress/gRPC/gRPC.md or https://github.com/open-telemetry/opentelemetry-specification/blob/master/work_in_progress/database/database.md - those attributes are removed from the originally copied specs.

Does this approach make sense for you?

@danielkhan
Copy link
Contributor Author

danielkhan commented May 21, 2019

@danielkhan I think it's a good idea to merge this document in and then start transferring pieces of it to the folder outside the work_in_progress folder.
[...]
Does this approach make sense for you?

Absolutely. That sounds like a good approach to get the ball rolling, @SergeyKanzhelev

@SergeyKanzhelev
Copy link
Member

I'm going to merge it as this is work in progress. Basically an accumulation of many documents. Documents that will require a bigger consensus will be placed outside of this folder.

@SergeyKanzhelev SergeyKanzhelev merged commit 776ae7a into open-telemetry:master May 21, 2019
@SergeyKanzhelev
Copy link
Member

@tylerbenson IMHO this work should be split into two parts.

@discostu105 perhaps there should be the separate issues for those separate discussions than

TuckTuckFloof pushed a commit to TuckTuckFloof/opentelemetry-specification that referenced this pull request Oct 15, 2020
rockb1017 pushed a commit to rockb1017/opentelemetry-specification that referenced this pull request Nov 18, 2021
Clarify versioning from experimental to stable
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.

None yet

7 participants