Define attribute requirement levels

[lmolkova]

Extracted general attribute requirement level from #2469

Changes

Adds terminology to be used by semantic convention to declare attribute presence requirements.

We currently have 4 vaguely defined levels across semantic conventions:

• always - all instrumentations MUST provide

• conditional which we use to specify when an attribute should be provided (but we use it inconsistently on optional and required attributes)
  e.g. rpc.service is recommended (but not required?)

     required:
       conditional: "No, but recommended"

  or http.retry_count that is optional and conditional

     required:
       conditional: If and only if a request was retried.

  or db.name which is required if available

     required:
       conditional: Required, if applicable.

• none, which means optional

• requires explicit enablement (e.g. HTTP headers)

This PR adds and clarifies the terminology:

• required - MUST provide
• conditional (naming suggestions are welcome) conditionally required - MUST IF <condition>
• recommended - SHOULD provide
• opt-in optional - MAY provide (when enabled)

Here's the corresponding tooling change: open-telemetry/build-tools#92

Related issues #2114.

[reyang]

LGTM.

[Oberon00]

Generally looks very sensible to me 👍, my only concern is about the MUST for transforming conditional/recommended into opt-in.

[carlosalberto]

Overall LGTM - let's address @tigrannajaryan concern though. Maybe we can discuss it in today's Spec call?

[tigrannajaryan]

@lmolkova Thank you for writing this PR. I think this is very important and we need to have it in the spec.

It has a big impact on all conventions and implementations, so please bear with me, I want to make sure the language is clear and precise enough in this section.

I think the PR now reads much better, there are only a few questions remaining that would be great to clarify before merging. Sorry for lots of comments :-)

I would also like to understand what others think about MUST vs SHOULD for the Required level.

[bogdandrutu]

Summary of the SIG discussion:

1. The "conditional" level is equivalent of SHOULD;
2. The "required" level is equivalent of MUST;

Bogdan's feedback:

1. Find a better name for "conditional";
2. Almost all current (already defined) "required" semantic conventions are "conditional";
3. @lmolkova Recommended in RFC 2119 is "SHOULD" and by their definition looks like "conditional" here.
4. @tigrannajaryan per RFC 2119 REQUIRED == MUST, I do believe that we should not use required everywhere like we do right now, but I would not make the change REQUIRED == SHOULD as suggested.

Bogdan's questions:

1. If the "summary" is correct, why not using directly the words in RFC 2119?

[tigrannajaryan]

@tigrannajaryan per RFC 2119 REQUIRED == MUST, I do believe that we should not use required everywhere like we do right now, but I would not make the change REQUIRED == SHOULD as suggested.

Interesting. I didn't know RFC defines "REQUIRED". I agree with this.

Once the new level definitions are accepted we will need to review all existing semantic conventions to make sure we don't unnecessarily use REQUIRED level.

[tigrannajaryan]

3. @lmolkova Recommended in RFC 2119 is "SHOULD" and by their definition looks like "conditional" here.

I think Conditional is different here because it has a "condition" associated with it.

[lmolkova]

Find a better name for "conditional";

How would everyone feel about Conditionally required?

@bogdandrutu

If the "summary" is correct, why not using directly the words in RFC 2119?

The summary is mostly correct, but not full:

• Required == MUST is just fine

• Conditionally required == MUST IF <CONDITION>, not really a SHOULD. If such an attribute is missing, telemetry loses a lot in quality

  • http.status_code MUST be provided if the response was received

  • net.host.port MUST be set if port is not default.

  • http.route MUST if available

  • At least one of process.executable.name, process.executable.path, process.command, process.command_line or process.command_args is required

    I still believe MUST works better here, but it doesn't really fit into RFC language.

• Recommended == SHOULD, unless disabled. Telemetry is mostly fine without it (e.g. http.user_agent could be redundunt on internal calls)

• Opt-in == SHOULD IF explicitly enabled

Considering these nuances RFC levels would not be descriptive enough.

[lmolkova]

Once the new level definitions are accepted we will need to review all existing semantic conventions to make sure we don't unnecessarily use REQUIRED level.

@tigrannajaryan and @bogdandrutu

I went through the existing semantic conventions (ones we have YAML for) and here's what I believe this change would mean for them:
lmolkova@8f53b12 - It would only change the attributes that we currently classify as required conditionally, but they are actually recommended.

We have 26 required: conditional attributes, which are mostly used to:

• describe constraints.any_of mostly for net.peer.name/ip substitutes
• skip if default (port, success response, empty string, etc)
• used with if available/applicable clause: db.name, db.statement (not disabled), net.transport, http.status_code, faas.invoked_region (comes with MUST if), messaging.destination_kind (set topic or queue if any of them)

I believe new terminology does not change semantics for them.

We have 26 required: always attributes and I believe they are safe to keep, the details:

• comes a lot on static things: os.type, service.name - also has MUST in desription, webengine_resource.name, db.system, messaging.system, rpc.system
• things defined in other specs: http.method, rpc.method, cloudevents.<many> (required in the CloudEvent spec)
• Specific implementations that can confidently require things: db.mongodb.collection, messaging.rocketmq has also a few required fields
• faas:
  • seems fine: faas.name (resource) and fass.invoked_name (span) - function name, faas.time, faas.trigger, faas.invoked_provider (cloud provider)
  • [might need to relax requirement, but apply to special-case convention and are probably fine] faas.document.collection, faas.document.operation, faas.document.time

So I believe this is a pretty minor change for current conventions and we already have good classification there, just some lack of terminology.

[bogdandrutu]

Required == MUST is just fine

+1

Conditionally Required == MUST IF , not really a SHOULD. If such an attribute is missing, telemetry loses a lot in quality

+1 I like the name.

Recommended == SHOULD, unless disabled. Telemetry is mostly fine without it (e.g. http.user_agent could be redundunt on internal calls)
Opt-in == SHOULD IF explicitly enabled

The RFC defines "recommended" as "should", without any caveat like "unless disabled". Are we mixing 2 "dimensions" here: 1. for devs to make enough effort to make these available in the instrumentation; 2. the capability for users of enabling/disabling these?

I would actually consider to change "Opt-In" to "OPTIONAL", and say that instrumentation plugins must offers way to "opt-out" for any recommended attributes, and to "opt-in' for any available "optional" attributes. the required and conditional required cannot be disabled, this way we separate the 2 concerns about what is available and what is configurable for the user.

Considering these nuances RFC levels would not be descriptive enough.

I agree, but I would use references to RFC when explain them, since is easier to understand.

[tigrannajaryan]

LGTM, great job.
I left some minor spelling comments.

[tigrannajaryan]

@lmolkova please resolve the conflicts.

[lmolkova]

@tigrannajaryan done, thank you!

[jamesmoessis]

This is super valuable work, thankyou for getting this through!