docs(context): Fix links, edit prose #2619

spencerwilson · 2021-11-12T01:55:38Z

Which problem is this PR solving?

Fix a couple dead links
Fix some styling ("JavaScript", "Node.js")
Add pointer to the Zone-based packages to help browser people
Elaborate on the two implementations provided by this package

Short description of the changes

Type of change

Please delete options that are not relevant.

This change requires a documentation update

^ I take it that since the updated docs aren't deployed anywhere, there's no other step needed.

Checklist:

Followed the style guidelines of this project
Unit tests have been added
Documentation has been updated

…y-js into sw/context-docs

linux-foundation-easycla · 2021-11-12T01:55:41Z

The committers are authorized under a signed CLA.

✅ Spencer Wilson (e86bb17, 7d9c732, 2993bf8, 1ed9290)

codecov · 2021-11-14T16:52:46Z

Codecov Report

Merging #2619 (5e9368a) into main (a824578) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2619   +/-   ##
=======================================
  Coverage   93.27%   93.27%           
=======================================
  Files         159      159           
  Lines        5444     5444           
  Branches     1141     1141           
=======================================
  Hits         5078     5078           
  Misses        366      366

packages/opentelemetry-context-async-hooks/README.md

vmarchaud · 2021-11-14T16:54:42Z

packages/opentelemetry-context-async-hooks/README.md


-### Limitations
+The former should be preferred over the latter as its implementation is substantially simpler than the latter's, and according to [Node.js docs](https://github.com/nodejs/node/blame/v17.1.0/doc/api/async_context.md#L42-L45),


AsyncLocalStorage isn't available for Node < 14.6 so we should document this there

It is also available for node ^12.17 and node ^13.10

According to the node 14 docs it was introduced in 13.10 so I think it is available in all v14 versions.

Please note that AsyncLocalStorage was added as experimental in 13.10 and it is still marked as experimental on 12 and 14 release lines (ignoring 13 and 15 for now as they are EOL anyway).

It was move out of experimental with 16.4.0.

There were several important fixes done in that area during the first 14.x releases resulting in using it only for >=14.8.0 in NodeTracerProvider.

I think all these fixes were backported to the 12.x line but most likely not with 12.17.0.

blumamir

LGTM.
Thanks for this great work

blumamir · 2022-01-06T08:24:47Z

@spencerwilson Can you please fix the conflict and run lint:fix so CI is green?

packages/opentelemetry-context-async-hooks/README.md

Flarna · 2022-01-07T12:36:52Z

packages/opentelemetry-context-async-hooks/README.md

+> While you can create your own implementation [of `AsyncLocalStorage`] on top of [`AsyncHook`], `AsyncLocalStorage` should be preferred as it is a performant and memory safe implementation that involves significant optimizations that are non-obvious to implement.
+
+`AsyncLocalStorage` is available in node `^12.17.0 || >= 13.10.0`, however `AsyncLocalStorageContextManager` is not enabled by default for node `<14.8.0` because of some important bugfixes which were introduced in `v14.8.0`.
+For more information see [nodejs/node#34573](https://github.com/nodejs/node/pull/34573).


I don't think this is the only relevant fix, it's the latest one.

spencerwilson · 2022-01-09T18:59:45Z

Thanks for all the reviews, everyone. I'll try to get this mergeable in the next few days.

dyladan · 2022-01-19T15:12:26Z

@spencerwilson any update?

spencerwilson · 2022-01-21T16:47:17Z

@dyladan apologies, I didn't get this in last weekend and weekends are likely the only time I'll have to do OTel things. I'm going to try to get this mergeable tomorrow or Sunday.

One question: Since my last commit I noticed that some Markdown docs in the JS repo(s?) are also published to opentelemetry.io (e.g., https://opentelemetry.io/docs/instrumentation/js/api/context/). Which type of links should I prefer to use in the Markdown: opentelemetry.io or github.com?

dyladan · 2022-01-21T16:48:59Z

One question: Since my last commit I noticed that some Markdown docs in the JS repo(s?) are also published to opentelemetry.io (e.g., opentelemetry.io/docs/instrumentation/js/api/context). Which type of links should I prefer to use in the Markdown: opentelemetry.io or github.com?

website. the docs in the repo are actually meant to be deprecated and removed in favor of the website we just haven't gotten around to it yet

…y-js into sw/context-docs

spencerwilson · 2022-01-24T06:23:03Z

Hrm, I was unable to run Markdown lint locally so I just went off of what I saw in the previous workflow logs. Had some trouble installing dependencies and lack experience with lerna to quickly know what's going on.

% npm i

> opentelemetry@0.1.0 postinstall
> npm run update-ts-references && npm run bootstrap


> opentelemetry@0.1.0 update-ts-references
> update-ts-references

updating tsconfigs
NO tsconfig.json for @opentelemetry/selenium-tests

> opentelemetry@0.1.0 bootstrap
> lerna bootstrap --hoist --nohoist='zone.js'

lerna notice cli v3.22.1
lerna info Bootstrapping 20 packages
lerna WARN EHOIST_ROOT_VERSION The repository root depends on semver@7.3.5, which differs from the more common semver@^7.3.5.
lerna WARN EHOIST_PKG_VERSION "@opentelemetry/sdk-trace-node" package depends on semver@^7.3.5, which differs from the hoisted semver@7.3.5.
lerna WARN EHOIST_PKG_VERSION "propagation-validation-server" package depends on @opentelemetry/api@^1.0.3, which differs from the hoisted @opentelemetry/api@~1.0.3.
lerna WARN EHOIST_PKG_VERSION "@opentelemetry/exporter-trace-otlp-grpc" package depends on @opentelemetry/api@^1.0.3, which differs from the hoisted @opentelemetry/api@~1.0.3.
lerna WARN EHOIST_PKG_VERSION "@opentelemetry/exporter-trace-otlp-http" package depends on @opentelemetry/api@^1.0.3, which differs from the hoisted @opentelemetry/api@~1.0.3.
lerna WARN EHOIST_PKG_VERSION "@opentelemetry/exporter-trace-otlp-proto" package depends on @opentelemetry/api@^1.0.3, which differs from the hoisted @opentelemetry/api@~1.0.3.
lerna WARN EHOIST_PKG_VERSION "@opentelemetry/exporter-jaeger" package depends on @opentelemetry/api@^1.0.3, which differs from the hoisted @opentelemetry/api@~1.0.3.
lerna WARN EHOIST_PKG_VERSION "@opentelemetry/exporter-zipkin" package depends on @opentelemetry/api@^1.0.3, which differs from the hoisted @opentelemetry/api@~1.0.3.
lerna WARN EHOIST_PKG_VERSION "@opentelemetry/selenium-tests" package depends on @opentelemetry/api@^1.0.3, which differs from the hoisted @opentelemetry/api@~1.0.3.
lerna WARN EHOIST_PKG_VERSION "@opentelemetry/selenium-tests" package depends on webpack@5.64.1, which differs from the hoisted webpack@4.46.0.
lerna info Installing external dependencies
lerna info hoist Installing hoisted dependencies into root
lerna info hoist Pruning hoisted dependencies
lerna info hoist Finished pruning hoisted dependencies
lerna ERR! npm install exited 1 in 'opentelemetry'
lerna ERR! npm install stderr:
npm ERR! code ERESOLVE
npm ERR! ERESOLVE unable to resolve dependency tree
npm ERR!
npm ERR! While resolving: opentelemetry@0.1.0
npm ERR! Found: webpack@4.46.0
npm ERR! node_modules/webpack
npm ERR!   webpack@"4.46.0" from the root project
npm ERR!
npm ERR! Could not resolve dependency:
npm ERR! peer webpack@"^5.1.0" from terser-webpack-plugin@5.2.5
npm ERR! node_modules/terser-webpack-plugin
npm ERR!   terser-webpack-plugin@"5.2.5" from the root project
npm ERR!
npm ERR! Fix the upstream dependency conflict, or retry
npm ERR! this command with --force, or --legacy-peer-deps
npm ERR! to accept an incorrect (and potentially broken) dependency resolution.

…y-js into sw/context-docs

spencerwilson · 2022-01-27T04:07:57Z

A couple flaky tests, looks like:

  1) OTLPTraceExporter - node with TLS, without metadata
       export
         should export spans:

      Uncaught AssertionError [ERR_ASSERTION]: resource doesn't exist
      + expected - actual

      -false
      +true
      
      at Timeout._onTimeout (test/OTLPTraceExporter.test.ts:175:18)
      at listOnTimeout (internal/timers.js:554:17)
      at processTimers (internal/timers.js:497:7)

  2) OTLPTraceExporter - node without TLS, without metadata
       export
         should export spans:

      Uncaught AssertionError [ERR_ASSERTION]: resource doesn't exist
      + expected - actual

      -false
      +true
      
      at Timeout._onTimeout (test/OTLPTraceExporter.test.ts:175:18)
      at listOnTimeout (internal/timers.js:554:17)
      at processTimers (internal/timers.js:497:7)

I've merged main to get a few more commits but none look like they'll influence these two tests.

*   81626216 (HEAD -> sw/context-docs) Merge branch 'main' of https://github.com/open-telemetry/opentelemetry-js into sw/context-docs
|\
| * e1c32b2d (upstream/main) feat(views): add FilteringAttributesProcessor (#2733)
| * 8daaddc6 feat(sdk-metrics-base): document and export basic APIs (#2725)
| * 63cff1cb chore: update actions/checkout to v2 (#2715)
* | 6e5ab03a (origin/sw/context-docs) docs(lint): maybe fix things

dyladan · 2022-01-27T14:58:54Z

Flaky tests aren't your fault

spencerwilson added 3 commits June 13, 2021 15:44

fix pkgs-that-break-ah link

e86bb17

doc(context): Fix links, edit prose

7d9c732

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

2993bf8

…y-js into sw/context-docs

spencerwilson requested a review from a team November 12, 2021 01:55

restore newline

1ed9290

vmarchaud added the document Documentation-related label Nov 14, 2021

vmarchaud reviewed Nov 14, 2021

View reviewed changes

packages/opentelemetry-context-async-hooks/README.md Show resolved Hide resolved

vmarchaud reviewed Nov 14, 2021

View reviewed changes

blumamir approved these changes Jan 6, 2022

View reviewed changes

Merge branch 'main' into sw/context-docs

4f820cb

dyladan reviewed Jan 6, 2022

View reviewed changes

packages/opentelemetry-context-async-hooks/README.md Show resolved Hide resolved

Update packages/opentelemetry-context-async-hooks/README.md

85a4319

Flarna reviewed Jan 7, 2022

View reviewed changes

vmarchaud approved these changes Jan 8, 2022

View reviewed changes

spencerwilson added 3 commits January 22, 2022 11:44

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

34ece4b

…y-js into sw/context-docs

docs(context-async-hooks): copy changes

ca416e3

docs(lint): maybe fix things

6e5ab03

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

8162621

…y-js into sw/context-docs

vmarchaud requested review from dyladan and Flarna January 29, 2022 08:53

Merge branch 'main' into sw/context-docs

5e9368a

vmarchaud merged commit ce5f7f7 into open-telemetry:main Jan 31, 2022

dyladan mentioned this pull request Mar 16, 2022

chore: release proposal 1.1.0 #2838

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs(context): Fix links, edit prose #2619

docs(context): Fix links, edit prose #2619

spencerwilson commented Nov 12, 2021 •

edited

Loading

linux-foundation-easycla bot commented Nov 12, 2021 •

edited

Loading

codecov bot commented Nov 14, 2021 •

edited

Loading

vmarchaud Nov 14, 2021

blumamir Jan 6, 2022 •

edited

Loading

dyladan Jan 6, 2022

Flarna Jan 7, 2022 •

edited

Loading

blumamir left a comment

blumamir commented Jan 6, 2022

Flarna Jan 7, 2022

spencerwilson commented Jan 9, 2022

dyladan commented Jan 19, 2022

spencerwilson commented Jan 21, 2022 •

edited

Loading

dyladan commented Jan 21, 2022

spencerwilson commented Jan 24, 2022

spencerwilson commented Jan 27, 2022

dyladan commented Jan 27, 2022


		### Limitations
		The former should be preferred over the latter as its implementation is substantially simpler than the latter's, and according to [Node.js docs](https://github.com/nodejs/node/blame/v17.1.0/doc/api/async_context.md#L42-L45),

docs(context): Fix links, edit prose #2619

docs(context): Fix links, edit prose #2619

Conversation

spencerwilson commented Nov 12, 2021 • edited Loading

Which problem is this PR solving?

Short description of the changes

Type of change

Checklist:

linux-foundation-easycla bot commented Nov 12, 2021 • edited Loading

codecov bot commented Nov 14, 2021 • edited Loading

Codecov Report

vmarchaud Nov 14, 2021

Choose a reason for hiding this comment

blumamir Jan 6, 2022 • edited Loading

Choose a reason for hiding this comment

dyladan Jan 6, 2022

Choose a reason for hiding this comment

Flarna Jan 7, 2022 • edited Loading

Choose a reason for hiding this comment

blumamir left a comment

Choose a reason for hiding this comment

blumamir commented Jan 6, 2022

Flarna Jan 7, 2022

Choose a reason for hiding this comment

spencerwilson commented Jan 9, 2022

dyladan commented Jan 19, 2022

spencerwilson commented Jan 21, 2022 • edited Loading

dyladan commented Jan 21, 2022

spencerwilson commented Jan 24, 2022

spencerwilson commented Jan 27, 2022

dyladan commented Jan 27, 2022

spencerwilson commented Nov 12, 2021 •

edited

Loading

linux-foundation-easycla bot commented Nov 12, 2021 •

edited

Loading

codecov bot commented Nov 14, 2021 •

edited

Loading

blumamir Jan 6, 2022 •

edited

Loading

Flarna Jan 7, 2022 •

edited

Loading

spencerwilson commented Jan 21, 2022 •

edited

Loading