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: Add new baggage span processor package #2114

Merged
merged 28 commits into from
May 2, 2024

Conversation

MikeGoldsmith
Copy link
Member

@MikeGoldsmith MikeGoldsmith commented Apr 18, 2024

Which problem is this PR solving?

Adds new baggage span processor as discussed in the following issue:

I've added to /packges directory - let me know if you'd prefer it somewhere else.

Short description of the changes

  • Adds new baggage span processor package

@MikeGoldsmith MikeGoldsmith requested a review from a team April 18, 2024 10:38
@MikeGoldsmith MikeGoldsmith changed the title Add new baggage span processor package feat: Add new baggage span processor package Apr 18, 2024
Copy link

codecov bot commented Apr 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.12%. Comparing base (dfb2dff) to head (ae60cae).
Report is 103 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2114      +/-   ##
==========================================
+ Coverage   90.97%   95.12%   +4.14%     
==========================================
  Files         146       10     -136     
  Lines        7492      697    -6795     
  Branches     1502      143    -1359     
==========================================
- Hits         6816      663    -6153     
+ Misses        676       34     -642     

see 137 files with indirect coverage changes

@Flarna
Copy link
Member

Flarna commented Apr 18, 2024

I think .github/component_owners.yml should be also updated
Not 100% sure about .github/component-label-map.yaml and release-please-config.json

Usually the opentelemetry- prefix for the folder is not used anymore for new stuff (see winston-transport).

@MikeGoldsmith
Copy link
Member Author

Thanks for the review @Flarna - I've updated the directory structure and fixed the readme title.

@MikeGoldsmith
Copy link
Member Author

Thanks for the suggestions @hectorhdzg - I've made some updates. Let me know what you think.

Copy link
Member

@JamieDanielson JamieDanielson left a comment

Choose a reason for hiding this comment

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

Thanks @MikeGoldsmith ! I've added a few suggestions for changes.

packages/baggage-span-processor/package.json Outdated Show resolved Hide resolved
packages/baggage-span-processor/README.md Outdated Show resolved Hide resolved
packages/baggage-span-processor/README.md Outdated Show resolved Hide resolved
packages/baggage-span-processor/package.json Outdated Show resolved Hide resolved
packages/baggage-span-processor/package.json Outdated Show resolved Hide resolved
@JamieDanielson
Copy link
Member

We'll also want to add to release-please-config.json based on the one check:

Could not find "packages/baggage-span-processor" entry in release-please-config.json. If you are adding a new package. Add following to "packages" object
"packages/baggage-span-processor": {},

Co-authored-by: Jamie Danielson <jamieedanielson@gmail.com>
@MikeGoldsmith
Copy link
Member Author

Thanks for review @JamieDanielson - I've worked through your feedback and suggestions. I think this is ready for another review.

Copy link
Member

@hectorhdzg hectorhdzg left a comment

Choose a reason for hiding this comment

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

LGTM

…b into mike/baggage-span-processor

# Conflicts:
#	.release-please-manifest.json
Copy link
Member

@blumamir blumamir left a comment

Choose a reason for hiding this comment

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

I think many users can benefit from this capability, thank you.

Added 2 non-blocking suggestions / discussions which I think might worth consideration so that we don't end up introducing breaking changes in the future or promoting something that might be unsafe by default.

@MikeGoldsmith current PR already add a useful feature so let me know if you prefer not to address those at the moment.

@MikeGoldsmith
Copy link
Member Author

@Flarna @hectorhdzg @blumamir Is there anything else you would like to see before this PR is accepted?

@MikeGoldsmith
Copy link
Member Author

Ping @open-telemetry/javascript-maintainers - looking for further feedback on this PR.

@blumamir blumamir merged commit 695177b into open-telemetry:main May 2, 2024
15 checks passed
@MikeGoldsmith MikeGoldsmith deleted the mike/baggage-span-processor branch May 2, 2024 14:05
@dyladan dyladan mentioned this pull request May 2, 2024
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.

Request for new component: Baggage Span Processor
5 participants