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

Typed Spans: Create RFC from current WIP proposal. #25

Closed
wants to merge 2 commits into from

Conversation

z1c0
Copy link
Contributor

@z1c0 z1c0 commented Aug 21, 2019

Since the "work in progress" folder is about to get removed from the repository (open-telemetry/opentelemetry-specification#225), the proposal concerning "typed spans" will be moved from there into the RFC repo.

See original discussion: open-telemetry/opentelemetry-specification#14

@z1c0 z1c0 changed the title Create RFC from current WIP proposal. Typed Spans: Create RFC from current WIP proposal. Aug 21, 2019
## Proposal
* Add a field `CanonicalType` that contains the type of span
* Define mandatory and optional attributes per span type
* Provide an API that supports creating typed spans and ensures that at least all
Copy link
Member

Choose a reason for hiding this comment

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

I would really like to see an actual proposal of the API for doing this. Especially if it was already POCed in Node, what's preventing it from becoming a part of this RFC? Without the API proposal this current document provides little value, since it's not directly usable.

Choose a reason for hiding this comment

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

Should we make language-specific RFCs? Not all languages support the same capabilities which makes writing specs for APIs a challenge. We can, however, link to the language spec files or should we rather use same pseudo code?

@AloisReitbauer
Copy link

Beyond the discussion above we should move these files over. This is just an administrative task rather than any substantial change to the spec. @tedsuo and @bogdandrutu can you please approve.

@lizthegrey
Copy link
Member

I think this proposal is missing an important element: declaring which parts of the attribute namespace are "reserved" for typed span data, which are for user attributes, etc. (e.g. can we declare that regardless of the type of the span, that most application data should be, for instance, prefixed with app.[appname].?

Copy link
Contributor

@tedsuo tedsuo left a comment

Choose a reason for hiding this comment

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

In subsequent conversations, we've moved from a single span type to composing semantic conventions as a mechanism for recording this information.

As requested, I'm approving this so that we can record it as proposed so that we can move it from the spec repo.

@discostu105
Copy link
Member

I believe this PR is a little bit ambiguous as it is. IMHO, it should be split into multiple standalone-pieces:

  1. Propose canonical type (with some concrete type-names)
  2. Semantic conventions per canonical type (these already exist, but maybe could use some refinements, better documenation, connection to concrete canonical types)
  3. Actual typed span APIs to help create spans that with proper attributes, that match semantic conventions for specific types.
  4. Fix "component" field inconsistency, because it partially overlaps with the "canonical type"
    • For HTTP, the definition is component:http, whereas for Database component:<name-of-db)
    • could also be part of 1.

@z1c0
Copy link
Contributor Author

z1c0 commented Sep 10, 2019

Closing this PR after some discussion.

  • We probably won't need a canonical type (we will check presence of attributes and apply a fixed resolution for multiple type spans, e.g. DB wins over HTTP).
  • Semantic conventions are discussed separately in the Data Formats meeting.
  • We should make a new OTEP/push for typed span APIs.

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.

6 participants