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: rename workspace dirs to match the base of the package name #2223

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

trentm
Copy link
Contributor

@trentm trentm commented May 17, 2024

This means dropping the 'opentelemetry-' prefix on a lot of workspace dirs,
and also fixing a couple odd balls:

  • opentelemetry-instrumentation-cassandra -> instrumentation-cassandra-driver
  • opentelemetry-test-utils -> contrib-test-utils

discussion

Update: see this comment below for an enumeration of options with Pros/Cons.

We've discussed at least once wanting to fix the package dir names to drop the "opentelemetry-" prefix. This is a draft PR to show what that might look like. Thoughts?

Pros:

  • It is easier/more obvious how to find the package dir when interested in a particular npm package. No more "sometimes there is an "opentelemetry-" prefix and sometimes not".

Cons:

  • This will break incoming deep links. (FWIW, I've added better "repository" entries to each package.json so links from npmjs.com will go to the correct dir, after the next publish.)
  • This may be a pain for currently open PRs at this time this is merged. I'm not actually sure. I haven't played with that.

Extreme version:

  • 🐴 What I really want is to have all packages be at ./packages/${name}, e.g. ./packages/instrumentation-express next to ./packages/resource-detector-aws. Then it is straightforward what the workspace dir is for a given package name (excluding "incubator" and "archive"d packages). Thoughts? If anyone is interested, I could create a draft PR to show what that looks like -- I have this renaming mostly scripted (https://gist.github.com/trentm/23591b5bde546a0f2f4b5b057b9d3ad9).

summary of dir renames

rename detectors/node/{opentelemetry-resource-detector-alibaba-cloud => resource-detector-alibaba-cloud}/
rename detectors/node/{opentelemetry-resource-detector-aws => resource-detector-aws}/
rename detectors/node/{opentelemetry-resource-detector-azure => resource-detector-azure}/
rename detectors/node/{opentelemetry-resource-detector-container => resource-detector-container}/
rename detectors/node/{opentelemetry-resource-detector-gcp => resource-detector-gcp}/
rename detectors/node/{opentelemetry-resource-detector-github => resource-detector-github}/
rename detectors/node/{opentelemetry-resource-detector-instana => resource-detector-instana}/
rename packages/{opentelemetry-test-utils => contrib-test-utils}/
rename packages/{opentelemetry-host-metrics => host-metrics}/
rename packages/{opentelemetry-id-generator-aws-xray => id-generator-aws-xray}/
rename packages/{opentelemetry-propagation-utils => propagation-utils}/
rename packages/{opentelemetry-redis-common => redis-common}/
rename packages/{opentelemetry-sql-common => sql-common}/
rename plugins/node/{opentelemetry-instrumentation-aws-lambda => instrumentation-aws-lambda}/
rename plugins/node/{opentelemetry-instrumentation-aws-sdk => instrumentation-aws-sdk}/
rename plugins/node/{opentelemetry-instrumentation-bunyan => instrumentation-bunyan}/
rename plugins/node/{opentelemetry-instrumentation-cassandra => instrumentation-cassandra-driver}/
rename plugins/node/{opentelemetry-instrumentation-connect => instrumentation-connect}/
rename plugins/node/{opentelemetry-instrumentation-dns => instrumentation-dns}/
rename plugins/node/{opentelemetry-instrumentation-express => instrumentation-express}/
rename plugins/node/{opentelemetry-instrumentation-fastify => instrumentation-fastify}/
rename plugins/node/{opentelemetry-instrumentation-generic-pool => instrumentation-generic-pool}/
rename plugins/node/{opentelemetry-instrumentation-graphql => instrumentation-graphql}/
rename plugins/node/{opentelemetry-instrumentation-hapi => instrumentation-hapi}/
rename plugins/node/{opentelemetry-instrumentation-ioredis => instrumentation-ioredis}/
rename plugins/node/{opentelemetry-instrumentation-knex => instrumentation-knex}/
rename plugins/node/{opentelemetry-instrumentation-koa => instrumentation-koa}/
rename plugins/node/{opentelemetry-instrumentation-memcached => instrumentation-memcached}/
rename plugins/node/{opentelemetry-instrumentation-mongodb => instrumentation-mongodb}/
rename plugins/node/{opentelemetry-instrumentation-mysql => instrumentation-mysql}/
rename plugins/node/{opentelemetry-instrumentation-mysql2 => instrumentation-mysql2}/
rename plugins/node/{opentelemetry-instrumentation-nestjs-core => instrumentation-nestjs-core}/
rename plugins/node/{opentelemetry-instrumentation-net => instrumentation-net}/
rename plugins/node/{opentelemetry-instrumentation-pg => instrumentation-pg}/
rename plugins/node/{opentelemetry-instrumentation-pino => instrumentation-pino}/
rename plugins/node/{opentelemetry-instrumentation-redis-4 => instrumentation-redis-4}/
rename plugins/node/{opentelemetry-instrumentation-redis => instrumentation-redis}/
rename plugins/node/{opentelemetry-instrumentation-restify => instrumentation-restify}/
rename plugins/node/{opentelemetry-instrumentation-router => instrumentation-router}/
rename plugins/node/{opentelemetry-instrumentation-winston => instrumentation-winston}/
rename plugins/web/{opentelemetry-instrumentation-document-load => instrumentation-document-load}/
rename plugins/web/{opentelemetry-instrumentation-long-task => instrumentation-long-task}/
rename plugins/web/{opentelemetry-instrumentation-user-interaction => instrumentation-user-interaction}/
rename plugins/web/{opentelemetry-plugin-react-load => plugin-react-load}/
rename propagators/{opentelemetry-propagator-instana => propagator-instana}/
rename propagators/{opentelemetry-propagator-ot-trace => propagator-ot-trace}/

trentm added 2 commits May 17, 2024 13:32
This means dropping the 'opentelemetry-' prefix on a lot of workspace dirs,
and also fixing a couple odd balls:
- instrumentation-cassandra -> instrumentation-cassandra-driver
- opentelemetry-test-utils -> contrib-test-utils
Copy link

codecov bot commented May 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.37%. Comparing base (dfb2dff) to head (5caea6b).
Report is 128 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2223      +/-   ##
==========================================
- Coverage   90.97%   90.37%   -0.61%     
==========================================
  Files         146      147       +1     
  Lines        7492     7502      +10     
  Branches     1502     1571      +69     
==========================================
- Hits         6816     6780      -36     
- Misses        676      722      +46     
Files Coverage Δ
...aba-cloud/src/detectors/AlibabaCloudEcsDetector.ts 97.67% <ø> (ø)
...urce-detector-alibaba-cloud/src/detectors/index.ts 100.00% <ø> (ø)
...detector-aws/src/detectors/AwsBeanstalkDetector.ts 95.65% <ø> (ø)
...ource-detector-aws/src/detectors/AwsEc2Detector.ts 97.91% <ø> (ø)
...ource-detector-aws/src/detectors/AwsEcsDetector.ts 96.90% <ø> (ø)
...ource-detector-aws/src/detectors/AwsEksDetector.ts 91.25% <ø> (ø)
...ce-detector-aws/src/detectors/AwsLambdaDetector.ts 100.00% <ø> (ø)
...or-aws/src/detectors/SemanticResourceAttributes.ts 100.00% <ø> (ø)
.../node/resource-detector-aws/src/detectors/index.ts 100.00% <ø> (ø)
...tor-azure/src/detectors/AzureAppServiceDetector.ts 96.66% <ø> (ø)
... and 41 more

... and 188 files with indirect coverage changes

@JamieDanielson
Copy link
Member

tldr; I'm a yes to all proposals here including the "extreme" version.

On the main update: consensus in the SIG meeting today was agreement in dropping opentelemetry- prefix for consistency. It seems relatively uncontroversial (aside from the open PRs breaking, which we'll help with as best we can to try to prevent too much disruption).

The renaming of the oddballs is also the right move. The general rule we're abiding by here is the package name should match the directory name. 👍

Finally, the "extreme" version of the directory changes. I am in favor of this but I think it's worthwhile to add some pros and cons to the possible approaches to take (everything in packages/ or all split by web/node/common, etc) to help folks consider the options.

@trentm
Copy link
Contributor Author

trentm commented May 22, 2024

@JamieDanielson Thanks for summarizing the SIG discussion!

@trentm
Copy link
Contributor Author

trentm commented May 22, 2024

options

  1. Do nothing. Don't change any of the dirs. I'm only listing this to be able to refer to it. I think there was general consensus that we do want to do some of the renamings.

  2. Drop opentelemetry- prefixes on dirs and rename to two oddballs. This is what this PR currently does.

    Pros:

    • It is more obvious what the dir is, given the package name.

    Cons:

    • It will cause a one-time break of incoming deep links. (This is mitigated somewhat by improving the "repository" package.json entries, so that links from npmjs.com will be to the correct subdir.)
    • Possible pain for currently open PRs.
    • Compared to Option 3, it is less obvious what the dir is for a package. A developer unfamiliar with the repo needs to poke around in six separate base dirs for packages.
  3. Move all packages to ./packages/{name}. This is what was called "extreme" above. In the SIG today there was favourable response to this, though not universal.

    Pros:

    • It is obvious what the dir is for a given contrib package.
    • (minor) Iterating over all packages is simpler, vs. needing to do this.

    Cons:

    • It will cause a one-time break of incoming deep links. (This is mitigated somewhat by improving the "repository" package.json entries, so that links from npmjs.com will be to the correct subdir.)
    • Possible pain for currently open PRs.
    • Compared to Option 2, the web and node instrumentations are not as obviously separated when listing a directory (i.e. there is no ls plugins/web/).
  4. Move all packages to a smaller set of dirs, with "node" vs "web" separation. For example, packages/node/{name} and packages/web/{name}. The web dir would hold the 4 packages in "plugins/web/" and "auto-instrumentations-node". This has all the same Pros and Cons of Option 2, plus:

    Pros:

    • It is easier for a developer interested primarily in web to identify the web instrumentations.

    Cons:

    • There is now a possible discussion needed for where a package is placed. E.g. do the propagators apply to both and need to go in "packages/common"?
    • Is there debate on the "packages/node" dir name, give possible applicability of some of the instrumentations to bun.js, deno, or other runtimes?

my opinions

I'm in favour of option 3.

I think the "which contrib packages are related to web/browser usage" could be solved by any of:

  • a section in the contrib repo README,
  • guidelines on the package description saying "web" or "browser" (building on Amir's recent docs: enhanced description for instrumentations in package.json #2202), or
  • even including "web" in the package names over time (e.g. if web instrumentations are to be re-implemented using events rather than spans, as I've heard; I may have heard wrong).

@trentm
Copy link
Contributor Author

trentm commented May 22, 2024

@MSNev Please let me know if that above fairly describes the concerns you had with option 3 in the SIG.

@MSNev
Copy link
Contributor

MSNev commented May 23, 2024

The is also a probable "middle" ground where there is no "node" folder but rather just /packages/<name> which would hold all of the "common" and "node" packages but we still have a /packages/web/<name> for the explicit web based packages as this would avoid the issue around the "where do common packages go"?

@JamieDanielson
Copy link
Member

  • (e.g. if web instrumentations are to be re-implemented using events rather than spans, as I've heard; I may have heard wrong).

These are the instrumentations currently slated to get rewritten to use events:

I assumed they would be new packages (vs updating the current), so this could be an opportunity to use a new name that includes "web" or "browser". I think @martinkuba would know more about whether I'm correct in that assumption.

@JamieDanielson
Copy link
Member

this could be an opportunity to use a new name that includes "web" or "browser".

✅ There was follow-up discussion on this and the decision was made that these new instrumentation package names should be prefixed or suffixed with web.

So the remaining question seems to be whether the web packages need to be in a separate subdirectory from everything else. Most attendees of two separate JS SIG meetings expressed interest in keeping everything in the packages directory without a subdirectory, as there didn't seem to be a strong enough reason to keep them separated. Wanted to offer up the opportunity for more web-focused folks to chime in on whether there is a strong objection to this, and a strong reasoning as to why web packages should be in a web subdirectory. @scheler @martinkuba @MSNev @pkanal tagging you as the folks most involved here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment