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

migrating doc draft #169

Merged
merged 14 commits into from
Jul 23, 2021
Merged

Conversation

jtmalinowski
Copy link
Contributor

@jtmalinowski jtmalinowski commented Jul 6, 2021

Description

Replace this with a description of the PR. Explain the problem it solves and the proposed solution.

Fixes #168

Type of change

Please delete options that are not relevant.

  • This change requires a documentation update

How Has This Been Tested?

  • Tested manually
  • Added automated tests

Checklist:

  • Changelogs have been updated
  • Unit tests have been added/updated
  • Documentation has been updated

@jtmalinowski jtmalinowski marked this pull request as ready for review July 20, 2021 17:22
@jtmalinowski jtmalinowski requested review from a team as code owners July 20, 2021 17:22
MIGRATING.md Outdated Show resolved Hide resolved
MIGRATING.md Outdated
Comment on lines 16 to 31
## Known limitations as compared to SignalFx Tracing Library

- Lowest supported version of NodeJS is `v8.5`, [see more information here](https://github.com/open-telemetry/opentelemetry-js#node-support)
- No auto-instrumentation for:
- `AdonisJS`
- `amqp10`
- `mongodb-core` ([because it's deprecated](https://github.com/mongodb-js/mongodb-core))
- `sails`
- Limited instrumentation for:
- `nest` - only manual insturmentation helpers, provided by community
- other notes on instrumentation:
- `express` instrumentation requires `http`/`https` instrumentation
- `bluebird` - context propagation only (via `async_hooks`)
- `q` - context propagation only (via `async_hooks`)
- `when` - context propagation only (via `async_hooks`)
- `socket.io` - provided by community (<https://github.com/aspecto-io/opentelemetry-ext-js/tree/master/packages/instrumentation-socket.io>)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we should talk of limitations. We want people to migrate, not to stick to the old stuff.

Can we change this section so that it states what the Splunk OTel dist for NodeJS is compatible with?

Copy link
Contributor Author

@jtmalinowski jtmalinowski Jul 22, 2021

Choose a reason for hiding this comment

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

Obviously we'll do our best to help the customers migrate, but let's make it clear what the pitfalls are.

MIGRATING.md Outdated
This Splunk Distribution of OpenTelemetry requires Node.js 8.5 or later.
If you're still using an earlier version of Node.js, continue using the SignalFx Tracing Library for Node.js.

## Equivalent configurations
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
## Equivalent configurations
## Configurations you can migrate

MIGRATING.md Outdated Show resolved Hide resolved
Comment on lines +46 to +47
With the exception of [explicitly listed limitations](#known-limitations) we aim to support all libraries supported by
signalfx-nodejs-tracing. To find an equivalent auto-instrumentation open <https://opentelemetry.io/registry/> and for
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto. Consider removing mention of limitations.

MIGRATING.md Outdated Show resolved Hide resolved
MIGRATING.md Outdated Show resolved Hide resolved
MIGRATING.md Outdated Show resolved Hide resolved
MIGRATING.md Outdated Show resolved Hide resolved
MIGRATING.md Outdated Show resolved Hide resolved
@jtmalinowski jtmalinowski force-pushed the chore/migrating-doc branch from 554758e to 622344d Compare July 22, 2021 15:21
[from `signalfx-nodejs-tracing`'s README](https://github.com/signalfx/signalfx-nodejs-tracing/#requirements-and-supported-software)
search by the name of the library in the registry.

For example, if you'd like to migrate instrumentation for `mysql`, go to
Copy link
Contributor

Choose a reason for hiding this comment

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

It's OK to keep it like that then

Copy link
Contributor

@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.

Awesome job! Mostly nits I don't feel strongly about, suggesting changes in limitations section and mentioning HTTP inst requirement for koa and hapi as well.

MIGRATING.md Outdated
- No auto-instrumentation for:
- `AdonisJS`
- `amqp10`
- `mongodb-core` ([because it's deprecated](https://github.com/mongodb-js/mongodb-core))
Copy link
Contributor

Choose a reason for hiding this comment

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

I would stress that MongoDB driver auto instrumentation is still supported. They just reogranized the client lib.

MIGRATING.md Outdated
- Limited instrumentation for:
- `nest` - only manual insturmentation helpers, provided by community
- other notes on instrumentation:
- `express` instrumentation requires `http`/`https` instrumentation
Copy link
Contributor

Choose a reason for hiding this comment

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

Same applies for koa and hapi for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh... and it's duplicated few lines below.

MIGRATING.md Outdated

Because the SignalFx Tracing Library for NodeJS uses OpenTracing and the Splunk Distribution
of OpenTelemetry for NodeJS uses OpenTelemetry, the semantic
conventions for span tag names change when you migrate. For more information,
Copy link
Contributor

Choose a reason for hiding this comment

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

.. and the span names as well! There are different guidelines for that in OTel.

MIGRATING.md Outdated Show resolved Hide resolved
MIGRATING.md Show resolved Hide resolved
MIGRATING.md Outdated Show resolved Hide resolved
MIGRATING.md Outdated Show resolved Hide resolved
MIGRATING.md Outdated Show resolved Hide resolved
MIGRATING.md Outdated Show resolved Hide resolved
MIGRATING.md Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jul 23, 2021

Codecov Report

Merging #169 (9714e8b) into main (5a567d3) will decrease coverage by 2.55%.
The diff coverage is 35.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #169      +/-   ##
==========================================
- Coverage   87.06%   84.51%   -2.56%     
==========================================
  Files          13       14       +1     
  Lines         286      297      +11     
  Branches       67       69       +2     
==========================================
+ Hits          249      251       +2     
- Misses         37       46       +9     
Impacted Files Coverage Δ
examples/logs/index.js 0.00% <0.00%> (ø)
src/instrumentations/index.ts 100.00% <ø> (ø)
src/options.ts 98.38% <100.00%> (+0.05%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a567d3...9714e8b. Read the comment docs.

@jtmal-signalfx jtmal-signalfx merged commit 994c90c into signalfx:main Jul 23, 2021
jtmal-signalfx pushed a commit that referenced this pull request Aug 30, 2021
* Include TS types and esm in release
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.

Link to migration guide is broken
5 participants