Proposal: Migrate instrumentation-aws-sdk to opentelemetry-js-contrib

[willarmiros]

First off, I want to thank the maintainers of this repo for their hard work and fantastic contributions to the instrumentations in this repo and the OpenTelemetry project at large. I am from the AWS X-Ray team, and we have really enjoyed using many of the instrumentations here, particularly the AWS SDK one. We even officially recommend it for for AWS customers using OTel.

However as we bring our OpenTelemetry distro out of public preview and into GA, we are not able to rely on a third-party hosted package for AWS SDK instrumentation. We would like to offer assistance in maintaining it, and also hopefully exposure to an even larger customer base, while also allowing the Aspecto team to continue to have ownership of it. That is why we are proposing that the opentelemetry-instrumentation-aws-sdk be moved upstream to the opentelemetry-js-contrib repo. All current Aspecto maintainers would continue to be CODEOWNERS (or whatever equivalent we come up with for package ownership in contrib) in addition to some engineers from AWS.

As discussed in #93, I believe @blumamir is now an approver in contrib, and I would hope we could discuss adding another approver from this repo so you'd be able to continue to iterate quickly in contrib as well. Being in contrib and having the @opentelemetry namespace would also hopefully lend well-deserved discoverability and credibility to the project. I will leave it up to the maintainers to decide where the other instrumentations in this repo should reside, but as the only one tied to a vendor the AWS SDK one seems like the odd one out.

/cc @alolita @NathanielRN @anuraaga @dyladan

[habmic]

Hi @willarmiros, thank you for the kind words!
So far our decision to have a separate repository is based on both the control over the code and rate of releases, however, I can understand your concerns and see the benefit of moving to opentelemetry-js-contrib

I Will update you after our internal discussion

[dyladan]

OTel JS will be moving to independent releases for packages by using https://github.com/googleapis/release-please. We are also going to try to assign ownership of packages in the contrib repo which will allow those package owners to merge changes more quickly. We hope this combination of changes will alleviate concerns with respect to merge and release velocity.

[dyladan]

@habmic I created an issue to track and discuss that. Please take a look and let us know if it would be sufficient for your usecase open-telemetry/opentelemetry-js-contrib#605

[blumamir]

Hi Guys,

After internal discussion, we would like to upstream the aws-sdk to contrib.
The merge and deploy velocity are very important to us. Are there any documented requirements in contrib for the process of approving a PR with regards to roles, specifically in these topics:

• amount of time a PR should be open before merged
• number of approvers \ maintainers \ CODE OWNERS to review the PR for merging
• who can perform the merge to main?
• who is authorized to publish a new version of the instrumentation

reference to the document in sdk repo

There are a few workflows and dependencies that we use in this repo that are not implemented in contrib. We would like to discuss \ raise a flag regarding migrating them as well.

test-all-version (dev dependency)

We use the test-all-versions package to run tests on all supported versions of the instrumentation. By doing this, we verify that the supportedVersions which instrumentation patch are indeed supported. This had been proven many times in our experience to find that a PR breaks old versions support which resulted in either a fix or drop of support in old versions. In cases like aws-sdk where there are tons of versions, we run it on all latest versions and a sample of old versions.

The CI runs the test-all-versions script for each PRs only on changed package using the lerna run --since option.

Daily Test Run in CI

In case a new version of an instrumented package is published, which doesn't play well with a plugin implementation we would like to know about it as soon as possible before code fails in production. We run a daily CI job on all supported node versions that execute the tests and alerts to slack in case of errors. This had proven in the past to find issues.

instrumentation-mocha (dev dependency)

Like contrib repo, we use mocha as a test runner. We instrument the mocha framework so that each test creates a trace where the entry span identifies the test (with attributes for name, suites, retry number, duration - test run time, if the test passed or not, and the error in case of failure). Spans created during test run are all belong to the same trace, which we can then export to jaeger and use to debug or visualize the tests traces while developing.
The instrumentation-mocha package is still needs some polishing.

Pre Release

We have a dedicated ci workflow to publish prerelease version of an instrumentation directly from the feature branch. This enables us to install it on our system and verify that nothing fails in a real production system before we merge it to master, publish it, and ship it to clients.

opentelemetry-instrumentation-testing-utils (dev dependency)

Our instrumentation testing depends on this package which manages some of the state involved in setups of testing instrumentations which I found to cause issues and code smells.

opentelemetry-propagation-utils (non-dev dependency)

The aws-sdk instrumentation depends on this package which can either stay in ext-js or be migrated as well to contrib. It handles the creation of process spans for messaging systems according to the specification.

We could also organize a zoom meeting or discuss those issues in the weekly meeting on Wednesday.

[willarmiros]

In general, for code ownership, the contrib repo will be using a custom solution described in open-telemetry/opentelemetry-js-contrib#605 to assign code owners, rather than the standard GitHub CODEOWNERS file. It's already been applied to some packages in open-telemetry/opentelemetry-js-contrib#606.

Here are some responses to address your initial concerns, if they all look good to @dyladan I can make a PR to add these to the upstream documentation. I believe the repos laid out in that merge requirements doc only apply to the core repo, not the contrib repo.

number of approvers \ maintainers \ CODE OWNERS to review the PR for merging

There is no official number or policy on this yet, but I discussed with @dyladan offline that one non-author code owner approval should be enough to merge.

who can perform the merge to main?

Only the OpenTelemetry repo maintainers, however my understanding is they will immediately merge PRs that only impact a particular component and have an approval from that component. I think @dyladan was discussing having the component owner tool adding an approved-by-owner label added so such PRs could be picked out and merged even more easily.

Maybe if we could verify that only code within the component scope was being modified, and it was approved by a component owner, then we could add a GH Action to automatically merge. But that might be a bit tricky.

amount of time a PR should be open before merged

I would hope that we could have a 1 business day SLA for approved-by-owner PRs to be merged, but ultimately that'd be up to the upstream maintainers.

who is authorized to publish a new version of the instrumentation

Per-component release automation is also being implemented in the OTel JS repos, per this PR: open-telemetry/opentelemetry-js#2393

Once added and tested out in the core repo, it will also be added to the contrib repo so that any package which has updates will automatically be included in the repo's next release. It is not yet clear how often these releases would occur, and is probably something we could discuss at the SIG.

Workflows/dev dependencies

Seems like there's quite a bit of tooling here, some of which could be beneficial to many upstream instrumentations, others which may not make sense to add. I agree with starting a conversation at a high level in the SIG, and we can schedule a separate call if needed. Of particular interest for other instrumentations I think are daily test runs, testing all (or a a good sample) supported versions, and your pre-release. My initial concern is for the length of the test-all-versions (e.g. almost 20 minutes for just the AWS SDK tests). But hopefully we can resolve that by tweaking the number of versions sampled and/or just living with longer CI times.

[blumamir]

Yesterday at the SIG meeting we discussed the issues above.
Will start migrating the package to contrib in the following days.

[blumamir]

First PR - upstream testing utils package used by aws-sdk instrumentation:
open-telemetry/opentelemetry-js-contrib#621

[blumamir]

Hi guys,

I'm kind of stack with the PR in contrib to add test-utils there. Is anyone available to look into this with me? or to brainstorm together and find the right solution?
This is a blocker for moving the instrumentations into the contrib repo.

Thanks

open-telemetry/opentelemetry-js-contrib#621

[willarmiros]

This is officially released upstream and can be found on NPM: https://www.npmjs.com/package/@opentelemetry/instrumentation-aws-sdk

Closing this issue, thanks again for all the hard work!