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

Verify resource attributes rename in schemas is correct #3245

Closed
carlosalberto opened this issue Feb 22, 2023 · 7 comments
Closed

Verify resource attributes rename in schemas is correct #3245

carlosalberto opened this issue Feb 22, 2023 · 7 comments
Assignees
@arminru
Labels
area:semantic-conventions Related to semantic conventions

Comments

@carlosalberto
Copy link
Contributor

As shown by #3190, using the spans format to rename attributes doesn't work for resources - instead, the following is needed. Whether that's fine or it's a limitation, we need to make it clear (i.e. see the attribute_map omission).

    spans:
      changes:
        - rename_attributes:
            attribute_map:
              http.user_agent: user_agent.original
    resources:
      changes:
        - rename_attributes:
          browser.user_agent: user_agent.original

Wdyt @tigrannajaryan ?
@tigrannajaryan
Copy link
Member

That is correct. There is no attribute_map for resources.changes.rename_attributes section. Here is an example https://github.com/open-telemetry/opentelemetry-go/blob/main/schema/v1.1/testdata/valid-example.yaml

The reason for that was that spans.changes.rename_attributes can have additional sub-key (other than attribute_map) and resources.changes.rename_attributes can't have any other sub-keys.

If we don't like this we can fix this and make resources uniform with spans. We have not yet released any schemas with resource changes so don't need to retroactively fix any old schema files and schema file format itself is still marked "Experimental" so we are allowed this breaking change.

@jsuereth thoughts?

@carlosalberto
Copy link
Contributor Author

The reason for that was that spans.changes.rename_attributes can have additional sub-key (other than attribute_map) and resources.changes.rename_attributes can't have any other sub-keys.

Thanks for the clarification. I'm fine with keeping them that way (we should add a note somewhere perhaps, just as clarification though).

@tigrannajaryan
Copy link
Member

I think for consistency and future-proofing it is best to change resource to use the same subkey.
I will submit some PRs.

@tigrannajaryan
Copy link
Member

The spec is correct. Nothing to fix here.

The schema checker is incorrect. I will submit bug fixes.

tigrannajaryan added a commit to tigrannajaryan/opentelemetry-go that referenced this issue Feb 24, 2023
@tigrannajaryan
Fix incorrect "all" and "resource" definition for Schema File
3e8767d 
The "all" and "resource" sections had incorrect definitions of "attribute_rename"
transform. It was missing the subkey "attribute_map".

This is a bug fix and makes the implementation compliant with the spec:
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/schemas/file_format_v1.1.0.md#resources-section

Related issue: open-telemetry/opentelemetry-specification#3245
@tigrannajaryan tigrannajaryan mentioned this issue Feb 24, 2023
Merged
@tigrannajaryan
Copy link
Member

Fix submitted in Go repo: open-telemetry/opentelemetry-go#3777

After this is merged will need to update the build tools.

tigrannajaryan added a commit to tigrannajaryan/opentelemetry-go that referenced this issue Feb 24, 2023
@tigrannajaryan
Fix incorrect "all" and "resource" definition for Schema File
d05d9da 
The "all" and "resource" sections had incorrect definitions of "attribute_rename"
transform. It was missing the subkey "attribute_map".

This is a bug fix and makes the implementation compliant with the spec:
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/schemas/file_format_v1.1.0.md#resources-section

Related issue: open-telemetry/opentelemetry-specification#3245
tigrannajaryan added a commit to tigrannajaryan/opentelemetry-go that referenced this issue Feb 27, 2023
@tigrannajaryan
Fix incorrect "all" and "resource" definition for Schema File
9a5f49f 
The "all" and "resource" sections had incorrect definitions of "attribute_rename"
transform. It was missing the subkey "attribute_map".

This is a bug fix and makes the implementation compliant with the spec:
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/schemas/file_format_v1.1.0.md#resources-section

Related issue: open-telemetry/opentelemetry-specification#3245
MrAlias added a commit to open-telemetry/opentelemetry-go that referenced this issue Feb 27, 2023
@tigrannajaryan @MrAlias
Fix incorrect "all" and "resource" definition for Schema File (#3777)
071d317 
The "all" and "resource" sections had incorrect definitions of "attribute_rename"
transform. It was missing the subkey "attribute_map".

This is a bug fix and makes the implementation compliant with the spec:
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/schemas/file_format_v1.1.0.md#resources-section

Related issue: open-telemetry/opentelemetry-specification#3245

Co-authored-by: Tyler Yahn <[email protected]>
@tigrannajaryan
Copy link
Member

Fix submitted to build tools: open-telemetry/build-tools#138

@arminru arminru added the area:semantic-conventions Related to semantic conventions label Feb 28, 2023
@arminru
Copy link
Member

arminru commented Feb 28, 2023

Fix released in https://github.com/open-telemetry/build-tools/releases/tag/v0.16.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@arminru arminru

Labels
area:semantic-conventions Related to semantic conventions
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@carlosalberto @tigrannajaryan @arminru