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

docs: update readmes to follow a standard pattern #960

Merged
merged 14 commits into from
Jun 9, 2022
Merged

docs: update readmes to follow a standard pattern #960

merged 14 commits into from
Jun 9, 2022

Conversation

pizzaz93
Copy link
Contributor

@pizzaz93 pizzaz93 commented Apr 7, 2022

Covers the node folder portion of the issue in #819.

Fixes #819

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 7, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: pizzaz93 / name: Peretz Cohen (ce04623)

@pizzaz93
Copy link
Contributor Author

pizzaz93 commented Apr 7, 2022

Validating the first file commit before propagating the edit to other files, @blumamir feel free to disregard for the moment.

@pizzaz93 pizzaz93 mentioned this pull request Apr 7, 2022
@rauno56
Copy link
Member

rauno56 commented Apr 8, 2022

I can confirm that's the idea of the issue! 👍

The wording has to be adjusted for the web bits.
Please also check if the new wording is correct saying each of the packages are included in the auto-instrumentations-node bundle.

@pizzaz93
Copy link
Contributor Author

pizzaz93 commented Apr 11, 2022

Meaning that the package is listed under the "Supported instrumentations" header?
https://www.npmjs.com/package/@opentelemetry/auto-instrumentations-node

What wording would we want for the web folder?

@dyladan @rauno56

@codecov
Copy link

codecov bot commented Apr 12, 2022

Codecov Report

Merging #960 (a395d2e) into main (125c2f7) will decrease coverage by 4.22%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #960      +/-   ##
==========================================
- Coverage   95.91%   91.68%   -4.23%     
==========================================
  Files          13      104      +91     
  Lines         856     5326    +4470     
  Branches      178     1083     +905     
==========================================
+ Hits          821     4883    +4062     
- Misses         35      443     +408     
Impacted Files Coverage Δ
...entation-aws-sdk/src/services/MessageAttributes.ts 90.32% <0.00%> (ø)
...extension-autoinjection/src/contentScript/index.ts 0.00% <0.00%> (ø)
...de/opentelemetry-instrumentation-hapi/src/utils.ts 100.00% <0.00%> (ø)
...try-instrumentation-restify/src/instrumentation.ts 89.32% <0.00%> (ø)
...pentelemetry-instrumentation-knex/src/constants.ts 100.00% <0.00%> (ø)
...ode/opentelemetry-instrumentation-dns/src/utils.ts 95.91% <0.00%> (ø)
.../opentelemetry-instrumentation-graphql/src/enum.ts 100.00% <0.00%> (ø)
...-instrumentation-nestjs-core/src/enums/NestType.ts 100.00% <0.00%> (ø)
...y-instrumentation-cassandra/src/instrumentation.ts 80.37% <0.00%> (ø)
...telemetry-instrumentation-fastify/src/constants.ts 100.00% <0.00%> (ø)
... and 81 more

@rauno56
Copy link
Member

rauno56 commented Apr 14, 2022

Meaning that the package is listed under the "Supported instrumentations" header? https://www.npmjs.com/package/@opentelemetry/auto-instrumentations-node

In the readme, yes, and under dependencies in the package.json.

What wording would we want for the web folder?

What would make sense in your opinion?

@pizzaz93
Copy link
Contributor Author

fs, tedious, router aren't included in https://www.npmjs.com/package/@opentelemetry/auto-instrumentations-node

I don't think I have sufficient context to suggest verbiage for the web folder content edits.

@pizzaz93 pizzaz93 marked this pull request as ready for review April 14, 2022 22:22
@pizzaz93 pizzaz93 requested a review from a team April 14, 2022 22:22
@rauno56
Copy link
Member

rauno56 commented Apr 26, 2022

fs, tedious, router aren't included in https://www.npmjs.com/package/@opentelemetry/auto-instrumentations-node

Adding those: #981
Thanks for going over them!

I don't think I have sufficient context to suggest verbiage for the web folder content edits.

I'd use something very analogous:

This module provides automatic instrumentation for [...], which may be loaded using the @opentelemetry/sdk-trace-web package.

If total installation size is not constrained, it is recommended to use the @opentelemetry/auto-instrumentations-web bundle with @opentelemetry/sdk-trace-web for the most seamless instrumentation experience.

@pizzaz93
Copy link
Contributor Author

pizzaz93 commented Apr 26, 2022

fs, tedious, router aren't included in https://www.npmjs.com/package/@opentelemetry/auto-instrumentations-node

Adding those: #981 Thanks for going over them!

I don't think I have sufficient context to suggest verbiage for the web folder content edits.

I'd use something very analogous:

This module provides automatic instrumentation for [...], which may be loaded using the @opentelemetry/sdk-trace-web package.
If total installation size is not constrained, it is recommended to use the @opentelemetry/auto-instrumentations-web bundle with @opentelemetry/sdk-trace-web for the most seamless instrumentation experience.

Thank you @rauno56, I've added the updated readme introduction syntax to the web folder as well. The PR is ready for review at this time.

Copy link
Member

@rauno56 rauno56 left a comment

Choose a reason for hiding this comment

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

Nice! Thanks and congrats for the first contribution!

@rauno56 rauno56 changed the title Updating Readme to follow a standard pattern docs: update readmes to follow a standard pattern May 3, 2022
@pizzaz93
Copy link
Contributor Author

pizzaz93 commented Jun 6, 2022

It's been two months since this PR was first filed, is this stuck on the dependent PR?

Copy link
Member

@blumamir blumamir left a comment

Choose a reason for hiding this comment

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

LGTM
Sorry about the delay and thanks for the reminder :)

@blumamir blumamir merged commit 29a3c56 into open-telemetry:main Jun 9, 2022
@pizzaz93 pizzaz93 deleted the readme-updates-pcohen branch June 10, 2022 17:00
@pizzaz93
Copy link
Contributor Author

Thank you @rauno56 and @blumamir.

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.

Word the readmes better
7 participants