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

fix(resources): extend ResourceAttributes interface to comply with spec #2924

Merged
merged 5 commits into from
Apr 30, 2022
Merged

fix(resources): extend ResourceAttributes interface to comply with spec #2924

merged 5 commits into from
Apr 30, 2022

Conversation

blumamir
Copy link
Member

There could be multiple ways to address this fix. I choose to use the interfaces from @opentelemetry/api but am open to other suggestions.

Which problem is this PR solving?

Fixes #2907

The current interface of ResourceAttributes supports only a subset of what is defined in the spec.
An array of primitive values are allowed in spec but are missing in the interface declaration in @opentelemetry/resources

Short description of the changes

Use the same type from @opentelemetry/api.
We should have used Attributes interface but since it's introduced in 1.1.0 I choose the SpanAttributes interface which will allow the SDK to work correctly with versions 1.0.x of the api.

This change only broader the definition of the ResourceAttributes interface so it is not expected to break anything.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

This is only change in typescript interface. Tested via CI run of current build / test / lint actions.

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@blumamir blumamir requested a review from a team April 24, 2022 11:15
@codecov
Copy link

codecov bot commented Apr 24, 2022

Codecov Report

Merging #2924 (eb309ac) into main (6436d85) will decrease coverage by 0.01%.
The diff coverage is n/a.

❗ Current head eb309ac differs from pull request most recent head 7447724. Consider uploading reports for the commit 7447724 to get more accurate results

@@            Coverage Diff             @@
##             main    #2924      +/-   ##
==========================================
- Coverage   93.04%   93.03%   -0.02%     
==========================================
  Files         183      183              
  Lines        6055     6055              
  Branches     1297     1297              
==========================================
- Hits         5634     5633       -1     
- Misses        421      422       +1     
Impacted Files Coverage Δ
...emetry-core/src/platform/node/RandomIdGenerator.ts 87.50% <0.00%> (-6.25%) ⬇️

@vmarchaud vmarchaud merged commit dc44b86 into open-telemetry:main Apr 30, 2022
@blumamir
Copy link
Member Author

I thought that this fix is not a breaking change, as we only broader the interface options.
However, after releasing 1.3.0, I get the following error about compatibility:

  Type 'import("/home/runner/work/opentelemetry-ext-js/opentelemetry-ext-js/node_modules/@opentelemetry/contrib-test-utils/node_modules/@opentelemetry/sdk-trace-base/build/src/export/ReadableSpan").ReadableSpan' is not assignable to type 'import("/home/runner/work/opentelemetry-ext-js/opentelemetry-ext-js/node_modules/@opentelemetry/sdk-trace-base/build/src/export/ReadableSpan").ReadableSpan'.
    The types of 'resource.attributes' are incompatible between these types.
      Type 'Attributes' is not assignable to type 'ResourceAttributes'.
        Index signatures are incompatible.
          Type 'AttributeValue' is not assignable to type 'string | number | boolean'.
            Type 'string[]' is not assignable to type 'string | number | boolean'.
              Type 'string[]' is not assignable to type 'string'.

I suspect that this change broke the compatibility between 1.2.0 and 1.3.0, as the interfaces are now different.

@Flarna
Copy link
Member

Flarna commented May 29, 2022

Looks like you have two instances of @opentelemetry/sdk-trace-base installed:
@opentelemetry/contrib-test-utils/node_modules/@opentelemetry/sdk-trace-base/build/src/export/ReadableSpan
@opentelemetry/sdk-trace-base/build/src/export/ReadableSpan

Maybe you pinned some older dependency?

@blumamir
Copy link
Member Author

Looks like you have two instances of @opentelemetry/sdk-trace-base installed: @opentelemetry/contrib-test-utils/node_modules/@opentelemetry/sdk-trace-base/build/src/export/ReadableSpan @opentelemetry/sdk-trace-base/build/src/export/ReadableSpan

Maybe you pinned some older dependency?

For reference: the issue is discussed under open-telemetry/opentelemetry-js-contrib#1042

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ResourceAttributes type does not comply with spec
5 participants