open-telemetry / semantic-conventions

Defines standards for generating consistent, accessible telemetry across a variety of domains
Apache License 2.0
244 stars 158 forks source link

Remove prefix in yaml, add policy check to block future usages (and minor cleanups) #1293

Closed lmolkova closed 1 month ago

lmolkova commented 1 month ago

Changes

Related to #1292 - this PR does not block anyone from using prefix, only changes the current pattern.

Merge requirement checklist

trisch-me commented 1 month ago

prefix is used in embed functionality, i.e. if I'm embedding geo.lat into client the resolved attribute should have full name as client.geo.lat

lmolkova commented 1 month ago

@trisch-me great point. This would be a blocker to #1292 and tooling changes https://github.com/open-telemetry/weaver/issues/263.

I still want to proceed with this PR since it does not stop someone from using prefix now (when necessary) or tooling from introducing any changes that can still leverage the prefix.

trisch-me commented 1 month ago

actually it does break it - as soon as prefix will be added because someone needs it the resolved registry with current weaver will double the prefix, so it will be prefix.prefix.attribute. I think this PR should go along together with weaver PR to NOT include prefix into resolved name anymore

lmolkova commented 1 month ago

actually it does break it - as soon as prefix will be added because someone needs it the resolved registry with current weaver will double the prefix, so it will be prefix.prefix.attribute.

whoever adds it, would remove it from the attribute id to not break things - nothing depends on prefix location today.

trisch-me commented 1 month ago

whoever adds it, would remove it from the attribute id to not break things

but then it is inconsistent. If we have decided to be explicit and provide prefix always in the id we need to update instrumentation as well so in case prefix would be there it will not break. Therefore I propose to merge this PR only together with/after weaver PR.

If you want I can create a PR in weaver which does this job but I'm against merging this PR until tooling is ready.

lmolkova commented 1 month ago

but then it is inconsistent. If we have decided to be explicit and provide prefix always in the id we need to update instrumentation as well so in case prefix would be there it will not break.

it's already inconsistent. Check out deprecated attributes in the registry. All the tooling is agnostic to where the prefix is today. Instrumentations use fully qualified name (prefix + id) since that's how tooling resolves name - the id/prefix is in most cases are not even visible beyond weaver/build-tools.

I'm ok waiting for the tooling changes. I'm trying to explain that this PR does not change status quo.

trisch-me commented 1 month ago

Instrumentations use fully qualified name (prefix + id) this is where I see things are breaking. Because if someone adds a prefix back because it's needed for other functionality, all id's in that group will be wrong or should be updated again to not have prefix in them. So better to remove dependency on prefix for name resolution

lmolkova commented 1 month ago

discussed at the tooling wg call

@trisch-me I want to double-check if you're ok with it. The embed design does not seem depend on the prefix anymore (#1264), but If it we end up needing it, we can always roll back this PR (partially or fully) - this is another benefit of enforcing it in the policy for now and not (yet) in the tooling.