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

chore: AWS SDK min version 3.0 #1166

Closed
wants to merge 1 commit into from

Conversation

arielvalentin
Copy link
Contributor

@arielvalentin arielvalentin commented Mar 30, 2022

Bumps auto-instrumentation version to 3.0, which is the minimum supported version as of 2021-11-21

We are announcing that version 2 of the AWS SDK For Ruby will enter maintenance mode on 11/20/2020.
Support for version 2 will end on 11/21/2021.

https://aws.amazon.com/blogs/developer/deprecation-schedule-for-aws-sdk-for-ruby-v2/

cc: @YanivD

Bumps auto-instrumentation version to 3.0, which is the minimum supported version as of 2021-11-21

> We are announcing that version 2 of the AWS SDK For Ruby will enter maintenance mode on 11/20/2020.
> Support for version 2 will end on 11/21/2021.

https://aws.amazon.com/blogs/developer/deprecation-schedule-for-aws-sdk-for-ruby-v2/
Copy link
Contributor

@plantfansam plantfansam left a comment

Choose a reason for hiding this comment

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

I had an informational question but this LGTM

@YanivD
Copy link
Member

YanivD commented Mar 31, 2022

@arielvalentin Thanks for the mention.

What is the motivation to drop support for versions 2.x.x? As far as I know the instrumentation works well also on versions 2.
I think if we can support those versions without heavy effort - it's worth it.

@arielvalentin
Copy link
Contributor Author

arielvalentin commented Mar 31, 2022

Supporting unmaintained version has come up before when someone requested we support Rails 4 and we made it clear that we would only support instrumentation for gems that receive maintenance updates.

In this case since the AWS-SDK instrumentation already demonstrated support for 2.0, they should lock the instrumentation to a specific version in their gemfile.

Once this PR is merged I can change how we perform version lookup. I will have some questions for you in a different PR.

@YanivD
Copy link
Member

YanivD commented Mar 31, 2022

I'm not sure I understand what is the motivation to drop support. Users might still use a deprecated version in real life projects.

IMO, it's valuable information and we should support version 2 if it's possible.

@ahayworth
Copy link
Contributor

It would be interesting to know if the v2 of the SDK is still immensely popular, perhaps.

@arielvalentin
Copy link
Contributor Author

Follow up question. Is this instrumentation tied to Seahorse versioning and not the AWS SDK version?

Should this instrumentation check compatibility against Seahorse instead of the AWS SDK.

@NathanielRN any thoughts or insights you can share from AWS team about this?

@ericmustin
Copy link
Contributor

ericmustin commented Mar 31, 2022

I think i'd like to get some clarification on this from the other maintainers or the GC on what policy we should enforce and what sort of discretion we have. This is something I've seen pop up on other tracing sdk's i've been involved with and I think it can be contentious, and I'd like to make sure everyone feels like they're heard.

Imo, if we have have some discretion here then i wouldn't be opposed to not restricting compatibility (don't bump min version), but perhaps dropping support (drop appraisals and tests against 2.x). There are tradeoffs to this (encouraging security anti patterns, potentially increased support tickets, tech debt for maintaining all the 2.x code, etc), but also benefits (more adoption). This is a tradeoff consideration I think is useful to think about on a case by case basis, and the considerations of rails 4.x are perhaps different than aws-sdk 2.x (just my two cents).

I also wouldn't be opposed to merging this PR but with the caveat of adding a support matrix in our documentation somewhere so users would know "if i'm running 2.x, i should pin to xyz instrumentation version, if i'm running 3.x, i should pin to a different xyz instrumentation version, and so on" This is an approach that worked well in my anecdotal experience, such as https://github.com/DataDog/dd-trace-rb/blob/master/docs/GettingStarted.md#integration-instrumentation. I think without this documentation it could be a very confusing experience to users.

Even if we don't have discretion and must drop support+compatiblity (i'm not sure if i'm using the correct terms but i think this gets my idea across), that compatibility matrix is probably a good thing to add.

Additionally, if we don't have discretion, then i think a followup item for us is to figure out a way to track this sort of thing across our instrumentations programmatically.

@fbogsany
Copy link
Contributor

The policy we discussed previously:

  1. Our auto-instrumentation will only explicitly support and test versions of the instrumented gem that are themselves officially maintained.
  2. We will feel free to use new features (e.g. Ruby language features) when they're supported by the minimum supported version.
  3. If a vendor (e.g. a Cloud vendor or an Observability vendor) commits to owning and maintaining auto-instrumentation for an older, unmaintained gem version, we'll support them in that. If they don't commit or don't address issues in a timely fashion, we will withdraw that support.

I think this is a pretty reasonable policy. It strikes the right balance for expectations and load on OpenTelemetry Ruby maintainers, and delegates ownership of "old" gem instrumentation to community members that benefit from it.

Without a clear policy like this, and strict adherence to it, we will have to re-litigate this on a case-by-case basis.

@fbogsany
Copy link
Contributor

Re: compatibility matrix, we either had or discussed this previously. IIRC it was not well maintained, and we opted to point at OpenTelemetry Registry instead. That is not ideal because it doesn't list supported versions.

@arielvalentin
Copy link
Contributor Author

The policy we discussed previously:

  1. Our auto-instrumentation will only explicitly support and test versions of the instrumented gem that are themselves officially maintained.
  2. We will feel free to use new features (e.g. Ruby language features) when they're supported by the minimum supported version.
  3. If a vendor (e.g. a Cloud vendor or an Observability vendor) commits to owning and maintaining auto-instrumentation for an older, unmaintained gem version, we'll support them in that. If they don't commit or don't address issues in a timely fashion, we will withdraw that support.

I think this is a pretty reasonable policy. It strikes the right balance for expectations and load on OpenTelemetry Ruby maintainers, and delegates ownership of "old" gem instrumentation to community members that benefit from it.

Without a clear policy like this, and strict adherence to it, we will have to re-litigate this on a case-by-case basis.

👍🏼 In this case then I think then the AWS SDK would fall under 3 since Aspecto (@YanivD and @blumamir) are effectively the maintainers and I think they have been good about keeping up with it.

Of course if AWS wouldn't mind we would appreciate it hint hint wink wink @NathanielRN

I will close this PR and preserve version compatibility in #1161

@arielvalentin arielvalentin deleted the aws-3.0 branch March 31, 2022 15:37
@NathanielRN
Copy link
Contributor

@arielvalentin Sorry for the delay! Would be happy to facilate setting up a meeting between you and the AWS SDK team if you’re interested but I don’t know the answer myself 🙂 Feel free to message me on Slack!

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.

7 participants