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

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

Merged
merged 6 commits into from
Aug 1, 2024

Conversation

lmolkova
Copy link
Contributor

@lmolkova lmolkova commented Jul 26, 2024

Changes

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

  • Changes all attribute definitions to use fully qualified attribute names and removes all prefixes
  • Add rego policy to detect and fail when prefix is used.
  • Removes (ignored) allow_custom_values on enums

Merge requirement checklist

@lmolkova lmolkova added the Skip Changelog Label to skip the changelog check label Jul 26, 2024
@lmolkova lmolkova requested review from a team July 26, 2024 22:06
@lmolkova lmolkova requested a review from tigrannajaryan as a code owner July 26, 2024 22:06
@lmolkova lmolkova requested a review from a team July 26, 2024 22:06
@trisch-me
Copy link
Contributor

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
Copy link
Contributor Author

@trisch-me great point. This would be a blocker to #1292 and tooling changes open-telemetry/weaver#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
Copy link
Contributor

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
Copy link
Contributor Author

lmolkova commented Jul 30, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

lmolkova commented Jul 30, 2024

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.

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

this seems like a good incremental improvement to me

@trisch-me
Copy link
Contributor

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
Copy link
Contributor Author

lmolkova commented Jul 31, 2024

discussed at the tooling wg call

  • it's important to make a consistent choice and, if the decision is to stop using prefix, to stop using it everywhere.
  • we can prohibit using prefix with rego policy - added one in this PR
  • schema and tooling updates can happen after migration to weaver.

@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.

@lmolkova lmolkova force-pushed the remove-prefix-everywhere branch from 4939b93 to 1cb0f1e Compare July 31, 2024 21:32
@lmolkova lmolkova changed the title Remove prefix in yaml and minor cleanups Remove prefix in yaml, add policy check to block future usages (and minor cleanups) Jul 31, 2024
Copy link
Contributor

@trisch-me trisch-me left a comment

Choose a reason for hiding this comment

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

thank you @lmolkova
With concrete goal to deprecate prefix I approve this PR

@lmolkova lmolkova merged commit a3cf57f into open-telemetry:main Aug 1, 2024
11 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog Label to skip the changelog check
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants