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

Issue 266 adapter => instrumentation rename #285

Conversation

ericmustin
Copy link
Contributor

Summary

This PR addresses #266

Basic Mapping

OpenTelemetry::Adapters => OpenTelemetry::Instrumentations
OpenTelemetry::Adapters::<GemName>::Adapter = > OpenTelemetry::Instrumentations::<GemName>::Instrumentation
OpenTelemetry::Instrumentation::Adapter => OpenTelemetry::Instrumentation::BaseInstrumentation

Lmk what you think, I tried to test this with circleci locally and things seem to pass but, its a large change and I did it mostly by hand so totally possible I have missed some places.

Notes

  • To make this a bit easier to review, the commits split things out between changes to /api, /sdk, /adapters (now /instrumentations), and misc config/build/testing sections.

  • Fwiw I don't love the naming conventions here, it feels a little confusing to me, if everyone was alright with Installer and that abided by the spec, instead of Instrumentation or Adapter , I would be happy to update. imo the docs often refer to a lower case "instrumentation" as the general unit of "code that provides traces for the gems and libraries", while the actual OpenTelemetry::Instrumentations::Sinatra::Adapter / OpenTelemetry::Instrumentations::Sinatra::Instrumentation class does the more specific work of attempting to install that code

@fbogsany
Copy link
Contributor

Sorry, I know this is a large change - the preferred name seems to be instrumentation rather than instrumentations. Technically, instrumentation is both singular and plural.

@ericmustin
Copy link
Contributor Author

@fbogsany no worries, so it should look like:

OpenTelemetry::Adapters => OpenTelemetry::Instrumentation
OpenTelemetry::Adapters::<GemName>::Adapter = > OpenTelemetry::Instrumentation::<GemName>::Instrumentation
OpenTelemetry::Instrumentation::Adapter => OpenTelemetry::Instrumentation::BaseInstrumentation

Just want to make sure I have it right

Copy link
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

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

Mostly LGTM. I think we are moving towards instrumentation (without an S) as the new terminology. There were still some comments with the plural form. There are also a few that have a redundant instrumentation. I think instrumentation adapter became instrumentation instrumentations in some cases. We just need to track down and smooth over the find and replace.

I have one comment about changing the environment variable format as well.

api/lib/opentelemetry/instrumentation.rb Outdated Show resolved Hide resolved
# OPENTELEMETRY_ADAPTERS_SINATRA_ENABLED. A value of 'false' will disable
# the adapter, all other values will enable it.
# environment variable name for OpenTelemetry::Instrumentation::Sinatra will be
# OPENTELEMETRY_INSTRUMENTATIONS_SINATRA_ENABLED. A value of 'false' will disable
Copy link
Member

Choose a reason for hiding this comment

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

The comment has INSTRUMENTATIONS. As an FYI, there is spec PR #666 that recomends forming environment language specific environment variables as OTEL_{LANGUAGE}_{FEATURE}. Which, would mean OTEL_RUBY_INSTRUMENTATION_SINATRA_ENABLED. We could consider making this change now so we only have to make it once, or track it separately.

Copy link
Contributor Author

@ericmustin ericmustin Jul 2, 2020

Choose a reason for hiding this comment

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

cursed spec number. Anyway, I've updated this to OTEL_RUBY_INSTRUMENTATION_SINATRA_ENABLED but, i'll admit, my approach is not the cleanest. I think it's a good approach, i'd be slightly concerned about this change (and all the others here) breaking things for folks using this library currently? Or, because this is in alpha, that's assumed that things may not be compatible from version to version?

api/lib/opentelemetry/instrumentation/registry.rb Outdated Show resolved Hide resolved
api/lib/opentelemetry/instrumentation/registry.rb Outdated Show resolved Hide resolved
instrumentation/all/README.md Outdated Show resolved Hide resolved
instrumentation/datadog-porting-guide.md Outdated Show resolved Hide resolved
instrumentation/datadog-porting-guide.md Outdated Show resolved Hide resolved
instrumentation/excon/lib/opentelemetry/instrumentation.rb Outdated Show resolved Hide resolved
instrumentation/excon/README.md Outdated Show resolved Hide resolved
instrumentation/excon/README.md Outdated Show resolved Hide resolved
sdk/lib/opentelemetry/sdk/configurator.rb Outdated Show resolved Hide resolved
sdk/lib/opentelemetry/sdk/configurator.rb Outdated Show resolved Hide resolved
sdk/lib/opentelemetry/sdk/configurator.rb Outdated Show resolved Hide resolved
@fbogsany
Copy link
Contributor

fbogsany commented Jul 8, 2020

This LGTM, thanks @ericmustin! This was a big chunk of work - much appreciated. I ✅ but it's worth letting @mwear take one last 👀 before we merge, especially at the env var stuff.

Copy link
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks @ericmustin!

@mwear mwear merged commit 1497723 into open-telemetry:master Jul 8, 2020
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.

align opentelemetry instrumentation naming with specification
3 participants