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.

Sign up for GitHub

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

Jump to bottom

Fix referencing template attributes #206

Merged
merged 3 commits into from
Sep 28, 2023
Merged

Fix referencing template attributes #206

merged 3 commits into from
Sep 28, 2023

Conversation

AlexanderWert
Copy link
Member

In #186 there was an oversight that template-type attributes cannot be referenced in other semantic conventions.

This PR fixes this bug and extends the corresponding test with a referenced template attribute.

@AlexanderWert AlexanderWert requested review from a team September 27, 2023 06:18
@AlexanderWert AlexanderWert added the bug Something isn't working label Sep 27, 2023
@AlexanderWert
Copy link
Member Author

@open-telemetry/specs-semconv-approvers , @open-telemetry/specs-semconv-maintainers

PTAL and let's get this fix in. open-telemetry/semantic-conventions#208 depends on this fix.

@AlexanderWert AlexanderWert changed the title Fix referrencing template attributes Fix referencing template attributes Sep 27, 2023
@AlexanderWert AlexanderWert mentioned this pull request Sep 27, 2023
Merged
lmolkova
lmolkova approved these changes Sep 27, 2023
View reviewed changes
Copy link
Contributor

@lmolkova lmolkova left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, but wanted to make sure we also support including template attributes by extending another semconv like

- id: foo
  attributes:
    - id: template
      type: template[string]

- id: bar
  extends:  foo

@AlexanderWert
Copy link
Member Author

LGTM, but wanted to make sure we also support including template attributes by extending another semconv like

@lmolkova Good point! I fixed that now as well and extended the test to cover it.

@arminru arminru merged commit 58f5187 into open-telemetry:main Sep 28, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lmolkova lmolkova lmolkova approved these changes

@joaopgrassi joaopgrassi joaopgrassi approved these changes

@arminru arminru arminru approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AlexanderWert @lmolkova @joaopgrassi @arminru