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 Instana propagator #1081

Merged
merged 6 commits into from
Aug 9, 2022
Merged

Conversation

basti1302
Copy link
Contributor

@basti1302 basti1302 commented Jul 5, 2022

Which problem is this PR solving?

Similar to the AWS X-Ray propagator, this adds a vendor specific propagator for the vendor specific trace correlation headers used by Instana.

Short description of the changes

Add Instana propagator plus unit tests.

Checklist

  • Ran npm run test for the new package on the latest commit if applicable.

Remark: The CLA is not in place yet, I'm working on that (as mentioned in the other two PRs I put up recently). My thinking was that the missing CLA would not be a blocker for a review/discussion phase for this PR.

@basti1302 basti1302 requested a review from a team July 5, 2022 15:03
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 5, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: basti1302 / name: Bastian Krol (e62ed81)

@dyladan
Copy link
Member

dyladan commented Jul 5, 2022

This looks good to me. Please sign the CLA. Are you and @kirrg001 members of the org yet? If not, you need to become a member in order for the component owners automations to work (only org members can be assigned issues/PRs).

@codecov
Copy link

codecov bot commented Jul 5, 2022

Codecov Report

Merging #1081 (dcb04c8) into main (69740ab) will increase coverage by 0.16%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1081      +/-   ##
==========================================
+ Coverage   95.91%   96.07%   +0.16%     
==========================================
  Files          13       14       +1     
  Lines         857      892      +35     
  Branches      179      191      +12     
==========================================
+ Hits          822      857      +35     
  Misses         35       35              
Impacted Files Coverage Δ
...emetry-propagator-instana/src/InstanaPropagator.ts 100.00% <100.00%> (ø)

@basti1302
Copy link
Contributor Author

Hey Daniel, thanks for the review 🙇

Please sign the CLA.

Yup, working on that. IBM has a corporate CLA in place, it just involves a little process and red tape to get on the list of contributors, so it might take a bit.

Are you and @kirrg001 members of the org yet? If not, you need to become a member in order for the component owners automations to work (only org members can be assigned issues/PRs).

Not yet. I guess becoming member of the org comes after having the CLA? I'll take care of that when the CLA is in place.

@dyladan
Copy link
Member

dyladan commented Jul 5, 2022

Are you and @kirrg001 members of the org yet? If not, you need to become a member in order for the component owners automations to work (only org members can be assigned issues/PRs).

Not yet. I guess becoming member of the org comes after having the CLA? I'll take care of that when the CLA is in place.

No need for a CLA specifically, but usually org membership requires you to show community engagement. Code contributions obviously require a CLA but there are non-code options as well which don't. Donating a full component that you plan to maintain probably counts as enough to gain membership though.

@basti1302 basti1302 force-pushed the propagator-instana branch 3 times, most recently from cdc5d0d to a22158f Compare July 6, 2022 14:28
@basti1302
Copy link
Contributor Author

Are you and @kirrg001 members of the org yet? If not, you need to become a member in order for the component owners automations to work (only org members can be assigned issues/PRs).

We both have our CLA in place now. If I understood correctly, a pre-requisite to merging this would be to add both of us to the org, since our Github handles are now mentioned in .github/component_owners.yml. How do we go about being added to the org?

@vmarchaud
Copy link
Member

@basti1302 The different level of membership are defined here: https://github.com/open-telemetry/community/blob/main/community-membership.md, you need to open an issue like this one (i can't find the template though), you can put any maintainer of us maintainers (dyladan/me for example) and link your opened PRs

@dyladan
Copy link
Member

dyladan commented Jul 26, 2022

I'm happy to sponsor that membership

@basti1302
Copy link
Contributor Author

basti1302 commented Jul 26, 2022

Apologies for not being responsive here for a while, I was on vacation. I am a member now (Sergey beat you to it, Daniel ;-) ), and Katharina's request is here (and needs one more support vote) and has two votes.

@basti1302
Copy link
Contributor Author

Seeing the comment #1084 (review) on the resource detector PR, should I also add this component to the two release please config files? I was under the impression that release-please would automatically add entries there with the next release?

@dyladan
Copy link
Member

dyladan commented Jul 26, 2022

Seeing the comment #1084 (review) on the resource detector PR, should I also add this component to the two release please config files? I was under the impression that release-please would automatically add entries there with the next release?

Yes you should

Copy link
Contributor

@kirrg001 kirrg001 left a comment

Choose a reason for hiding this comment

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

LGTM

@basti1302
Copy link
Contributor Author

basti1302 commented Aug 9, 2022

I think this would be ready to be merged. Github says it needs a review from open-telemetry/javascript-approvers.

@basti1302 basti1302 force-pushed the propagator-instana branch from 5ceff6b to c19f4e2 Compare August 9, 2022 12:51
@Flarna
Copy link
Member

Flarna commented Aug 9, 2022

as far as I know approvals from @open-telemetry/javascript-maintainers are also needed here.

@dyladan
Copy link
Member

dyladan commented Aug 9, 2022

Can't merge a branch out of date with the base branch and don't have permission to update your branch

@basti1302 basti1302 force-pushed the propagator-instana branch from c19f4e2 to dcb04c8 Compare August 9, 2022 13:57
@basti1302
Copy link
Contributor Author

I have rebased this now.

@dyladan
Copy link
Member

dyladan commented Aug 9, 2022

Merging when tests pass

@dyladan dyladan merged commit d9546f8 into open-telemetry:main Aug 9, 2022
@dyladan dyladan mentioned this pull request Aug 9, 2022
@basti1302 basti1302 deleted the propagator-instana branch August 9, 2022 15:05
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.

5 participants