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

feat(otlp-transformer): consolidate scope/resource creation in transformer #4600

Merged
merged 8 commits into from
Apr 4, 2024

Conversation

pichlermarc
Copy link
Member

Which problem is this PR solving?

Resources and scopes creation are currently duplicated across all signals even though they're the same. This PR pulls them out into shared functions for all signals.

Replaces #4429

Copy link

codecov bot commented Apr 2, 2024

Codecov Report

Merging #4600 (6094b06) into main (e01f493) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4600      +/-   ##
==========================================
- Coverage   92.83%   92.83%   -0.01%     
==========================================
  Files         328      329       +1     
  Lines        9519     9526       +7     
  Branches     2050     2050              
==========================================
+ Hits         8837     8843       +6     
- Misses        682      683       +1     
Files Coverage Δ
...l/packages/otlp-transformer/src/common/internal.ts 100.00% <100.00%> (ø)
...mental/packages/otlp-transformer/src/logs/index.ts 100.00% <100.00%> (ø)
.../packages/otlp-transformer/src/metrics/internal.ts 100.00% <100.00%> (ø)
...packages/otlp-transformer/src/resource/internal.ts 100.00% <100.00%> (ø)
...ental/packages/otlp-transformer/src/trace/index.ts 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

@pichlermarc
Copy link
Member Author

FYI @legendecas this is the stale #4429 with the changes I suggested to make the workflows pass.

@pichlermarc pichlermarc marked this pull request as ready for review April 2, 2024 10:50
@pichlermarc pichlermarc requested a review from a team April 2, 2024 10:50
Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

I am fine with droppedAttributesCount since it exists in the proto. However, as the key attributes is optional, I think droppedAttributesCount could be optional as well.

@pichlermarc
Copy link
Member Author

I am fine with droppedAttributesCount since it exists in the proto. However, as the key attributes is optional, I think droppedAttributesCount could be optional as well.

@legendecas, ah sorry I took me a while to understand what you meant on the original PR. 😅

As it's optional in the proto, and we don't set it to anything other than 0, we might as well leave it out, you're right. I pushed a commit to change this (56cf720). This way we also don't need to adapt the tests.

@pichlermarc pichlermarc merged commit b78d443 into open-telemetry:main Apr 4, 2024
20 checks passed
@pichlermarc pichlermarc deleted the rmduplicate branch April 4, 2024 13:21
Zirak pushed a commit to Zirak/opentelemetry-js that referenced this pull request Sep 14, 2024
…ormer (open-telemetry#4600)

* [chore] consolidate scope/resource creation in transformer

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>

* fixup! [chore] consolidate scope/resource creation in transformer

* fixup! [chore] consolidate scope/resource creation in transformer

* chore: add changelog entry

* fixup! [chore] consolidate scope/resource creation in transformer

---------

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
Co-authored-by: Bogdan Drutu <bogdandrutu@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants