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

feat: Add new baggage span processor package #2114

Merged
merged 28 commits into from May 2, 2024
Merged

feat: Add new baggage span processor package #2114

merged 28 commits into from May 2, 2024

Conversation

MikeGoldsmith
Copy link
Member

@MikeGoldsmith MikeGoldsmith commented Apr 18, 2024 •
edited

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 as a code owner 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
@codecov Codecov
Copy link

codecov bot commented Apr 18, 2024 •
edited

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.

JamieDanielson
JamieDanielson reviewed Apr 19, 2024
View reviewed changes
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/src/baggage-span-processor.ts 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": {},

@MikeGoldsmith @JamieDanielson
Apply suggestions from code review
1d3a86d 
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.

hectorhdzg
hectorhdzg approved these changes Apr 22, 2024
View reviewed changes
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

@MikeGoldsmith MikeGoldsmith mentioned this pull request Apr 25, 2024
Merged
3 tasks
@MikeGoldsmith
Merge branch 'main' of github.com:honeycombio/opentelemetry-js-contri…
c59eaba 
…b into mike/baggage-span-processor

# Conflicts:
#	.release-please-manifest.json
blumamir
blumamir reviewed Apr 29, 2024
View reviewed changes
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.

packages/baggage-span-processor/README.md Show resolved Hide resolved
packages/baggage-span-processor/src/baggage-span-processor.ts Show resolved Hide resolved
This was referenced Apr 29, 2024
Open
Closed
@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
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Flarna Flarna Flarna approved these changes

@blumamir blumamir blumamir approved these changes

@hectorhdzg hectorhdzg hectorhdzg approved these changes

@JamieDanielson JamieDanielson Awaiting requested review from JamieDanielson

Assignees

@MikeGoldsmith MikeGoldsmith

@JamieDanielson JamieDanielson

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Request for new component: Baggage Span Processor
5 participants
@MikeGoldsmith @Flarna @JamieDanielson @blumamir @hectorhdzg