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: adapting yaml syntax for splitting attribute definition from usage #191

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

AlexanderWert
Copy link
Member

This is a proposal to get discussions started around step 2 from open-telemetry/semantic-conventions#197 (comment).

proposed model syntax changes summarized:

  • new entity namespace that is similar to existing semconv but:
    • omitting convtype, extends and constraints
    • requiring a prefix
    • attribute_definitions instead of attributes
  • new entity attribute_definitions that is similar to existing attributes but:
    • requiring id, type, brief and examples
    • omitting ref, requirement_level
  • changes on semconv:
    • omitting to specify a prefix (this will be covered in the namespace when defining the attributes)
  • changes on attributes:
    • forbid defining attributes, only ref to an attribute_definition is allowed

That's how it would look like in YAML files:

The attribute definition:

groups:
  - id: registry.http   <<- THIS is a namespace, because prefix is not allowed for a semconv anymore
    prefix: http            
    brief: 'This document defines semantic convention attributes in the HTTP namespace.'
    attribute_definitions:        <<- NEW: contains only definitions of attributes (without requirement_level, etc.)
      - id: request.body.size
        type: int
        brief: >
          The size of the request payload body in bytes. ...
        examples: 3495

The attribute usage in semantic conventions:

groups:
  - id: trace.http.common   <<- THIS is a semconv (because prefix is not defined)
    extends: attributes.http.common
    type: attribute_group
    brief: 'This document defines semantic conventions for HTTP client and server Spans.'
    note: >
        These conventions can be used for http and https schemes
        and various HTTP versions like 1.1, 2 and SPDY.
    attributes:         <<- ATTRIBUTES here can only reference attribute definitions, they cannot be defined here anymore
      - ref: http.request.body.size
        requirement_level: required

Signed-off-by: Alexander Wert <alexander.wert@elastic.co>
@discostu105
Copy link
Member

At Dynatrace, we are developing something similar internally. We differentiate between common attribute definitions and specific model definitions (which mostly just reference common fields).

One thing we learned along the way:

  • There are attributes that are model specific (only apply to one model) and only really have meaning in that model. It would not make sense to define "globally". An example would be "name" or "billing_type".
  • Then there are attributes that are usable in a certain model domain, but not in other domains. E.g. There are many event definitions for a feature we call "workflow engine". A shared field there is called "workflow.owner". But this field would not make sense to define globally, as it might be too generic.

We solved these issues by allowing models to still define their own attributes, in case they are model-specific. Also, we allow "shared_field" definitions in model domains.

Not sure if these learnings apply to OTel in the same way as they did for us. Some of the issues can always be solved by proper prefixing / namespacing all attributes.

Copy link
Member

@joaopgrassi joaopgrassi left a comment

Choose a reason for hiding this comment

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

I like this proposal!

One question looking at your "usage example:

groups:
  - id: registry.http   <<- THIS is a namespace, because prefix is not allowed for a semconv anymore
    prefix: http            

You mentioned the id is the namespace because prefix is not allowed, but that is a "attribute registry" and not a semconv, was that a typo/mistake?

Because below, when you use it you refer it with the prefix: http so I'm a bit confused.

@@ -40,10 +40,15 @@ here in `syntax.md` should be considered more authoritative though. Please keep
All attributes are lower case.

```ebnf
groups ::= semconv
| semconv groups
groups ::= group
Copy link
Member

Choose a reason for hiding this comment

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

There's group(s) many times here and it makes me very confused 😅. Not sure what but I think it would be best if we name it differently. Maybe convention? And also couldn't namespace be attribute_registry? Something like:

groups ::= convention
       | convention groups

convention::= semconv
      | attribute_registry

attribute_registry ::= id brief [note] prefix [stability] [deprecated] attribute_definitions

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.

4 participants