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

Detect when the same attribute is referenced more than once #175

Open
lmolkova opened this issue May 24, 2024 · 5 comments
Open

Detect when the same attribute is referenced more than once #175

lmolkova opened this issue May 24, 2024 · 5 comments
Assignees
Labels
bug Something isn't working

Comments

@lmolkova
Copy link
Contributor

Describe the bug
build-tools had a check that prevented referencing the same attribute multiple times within one group.

e.g.

  - id: messaging.servicebus
    type: attribute_group
    extends: attributes.messaging.trace.minimal
    brief: >
      Attributes for Azure Service Bus
    attributes:
      - ref: messaging.operation.name
      - ref: messaging.operation.name
        brief: Azure Service Bus operation name.

caused an error like

Error parsing /source/trace/messaging.yaml:
Attribute id messaging.operation.name is already present at line 235 - @251:9 ('messaging.operation.name')

To Reproduce
See
open-telemetry/semantic-conventions@3c52fd0
and
https://github.com/open-telemetry/semantic-conventions/actions/runs/9230121422/job/25397692938?pr=1006

Expected behavior
table check done with weaver should detect it and complain (in the above CI check, it's still back-compat in semconv that raises an issue)

@jsuereth
Copy link
Contributor

Can we make this a policy in: open-telemetry/semantic-conventions#1014?

Or should we consider this a core aspect of the model we enforce?

@lmolkova
Copy link
Contributor Author

Can we make this a policy in: open-telemetry/semantic-conventions#1014 we make this a policy in: open-telemetry/semantic-conventions#1014?

I think it would be even better to define it as a policy in semconv

@lquerel
Copy link
Contributor

lquerel commented May 28, 2024

Seems to be a good candidate for the policy engine.

@jsuereth
Copy link
Contributor

Coming back to this - it DOES seem like the core model shouldn't allow two refs of the same thing. Going to mark this a bug but would like to either agree to fix in weaver or close as acceptable.

@lquerel
Copy link
Contributor

lquerel commented Nov 25, 2024

@jsuereth Yes, I agree. That should be in the Weaver core model, and not in the policies as I mentioned several months ago.

@jsuereth jsuereth self-assigned this Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants