Update build-tools version to 0.25.0 and add display_name to all registry attribute groups

[AlexanderWert]

Fixes #978

[AlexanderWert]

To make CI happy we need also: open-telemetry/build-tools#316

[trisch-me]

thanks for quick implementing it. @jsuereth could add this now into the final PR to populate correct name

[ChrsMark]

LGTM

[trisch-me]

One question though - should we enforce then a presence of this property for registry?

[AlexanderWert]

One question though - should we enforce then a presence of this property for registry?

yeah, I guess we could extend open-telemetry/build-tools#316 to include it in the validity checks

[lmolkova]

Could you please create an issue to update schema definition (or just update the schema definition) in the https://github.com/open-telemetry/build-tools/blob/main/semantic-conventions/semconv.schema.json to include it?

We want to move it to this repo #916, but it's not done yet

[noahfalk]

LGTM

[noahfalk]

@JamesNK (Just fyi - This looks like a minor cosmetic change)

[jsuereth]

One question though - should we enforce then a presence of this property for registry?

Once we get the rule-engine/checker working in weaver, this is a simple configuration for semantic-conventions. Let's see if we can get that prioritized as a the next feature we need from weaver for semconv.

[jsuereth]

Thanks for adding all of these.

We need to update both build-tools and weaver. I merged your build-tools change (will need a release) but we'll also need to update weaver.

Since the weaver PR merged, you should be able to actually update the weaver attribute_group.md.j2 template to use this value in this PR.

[AlexanderWert]

Could you please create an issue to update schema definition (or just update the schema definition) in the https://github.com/open-telemetry/build-tools/blob/main/semantic-conventions/semconv.schema.json to include it?

Done, see: open-telemetry/build-tools#317

[AlexanderWert]

Since the weaver PR merged, you should be able to actually update the weaver attribute_group.md.j2 template to use this value in this PR.

@jsuereth Done with the last commit.

[lmolkova]

@AlexanderWert I think you need to run make table-generation to re-generate registry MD files with display name.

[AlexanderWert]

@AlexanderWert I think you need to run make table-generation to re-generate registry MD files with display name.

That's not working yet, I'm getting a validation error from weaver. So, as @jsuereth mentioned above, we need to fix weaver validation first and release a new docker image, I guess.

@jsuereth What exactly needs to be done to fix the weaver validation? Is it just about adding display_name as a property in this struct?: https://github.com/open-telemetry/weaver/blob/261e4f42518fee67a00b1a81be4029a82fd19799/crates/weaver_semconv/src/group.rs#L21

[jsuereth]

@AlexanderWert Yes - I think that change (and propagation of its value) should be what you need to do in weaver.

There's also a TemplateGroup: https://github.com/open-telemetry/weaver/blob/main/crates/weaver_forge/src/registry.rs#L29

Once you have Group / TemplateGroup updated and wired - we can immediately start rendering this attribute in the registry.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[AlexanderWert]

The only thing is: Should we mark the new field as required? The Jinja template now assumes it is but if one attribute group misses it the md rendering will fail (I think).

Can we make it required only for registry attribute groups?

[trisch-me]

we can enforce through the policies that this attribute is required for registry

[joaopgrassi]

Can we make it required only for registry attribute groups?

Hum I think not, specially in the JSON schema but also on Weaver. What we can do is in the jinja template we check if display name exists and if not, we use the old behavior. I gave that a try and it worked for me locally.

[joaopgrassi]

we can enforce through the policies that this attribute is required for registry

Ah really? Then forget my comment above 👍

[trisch-me]

Then forget my comment above

but until we have it implemented we can just use the workaround you proposed

[joaopgrassi]

@AlexanderWert are you able to continue this work, or should one of us take over?

[AlexanderWert]

@AlexanderWert are you able to continue this work, or should one of us take over?

I'll update the PR with your proposal

[AlexanderWert]

@joaopgrassi updated this PR

I think we need both releasing and updating the weaver version AND build tools, right?

[joaopgrassi]

I think we need both releasing and updating the weaver version AND build tools, right?

Weaver for sure we need a new release. For build-tools I'm not sure.. but I assume we might (maybe bcs of the JSON schema?)

@lquerel can we have a new Weaver release?

[lmolkova]

Oh, I think we still need a build-tools release for this.

[lquerel]

I think we need both releasing and updating the weaver version AND build tools, right?

Weaver for sure we need a new release. For build-tools I'm not sure.. but I assume we might (maybe bcs of the JSON schema?)

@lquerel can we have a new Weaver release?

@joaopgrassi The release v0.5.0 of Weaver contains this feature.

[joaopgrassi]

Yes, we have the release and already updated Weaver version in semconv. @AlexanderWert will update the PR because it needs some changes before merging.

[AlexanderWert]

@joaopgrassi @lmolkova @lquerel
Updated my PR.

PTAL, I also changed the order of the generated groups to make sure deprecated attributes are always listed at the end (instead of pure alphabetical order)

[AlexanderWert]

@lmolkova The compatibility check still fails.

That has been actually fixed in these PRs, but we did not have a build-tools release since then:
https://github.com/open-telemetry/build-tools/pulls?q=is%3Apr+display_name+is%3Aclosed

So, I guess we need to do a new release of the build tools and update it here.

[lmolkova]

@AlexanderWert build-tools v0.25.0 were just published, could you try updating? Thanks!