Add OpenTelemetry span processor #651
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
luismiramirez
approved these changes
May 2, 2022
tombruijn
approved these changes
May 2, 2022
packages/nodejs/.changesets/add-opentelemetry-span-processor.md
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
|
I've bumped the agent and extension, and fixed the build. |
unflxw
approved these changes
May 6, 2022
| /** | ||
| * OpenTelemetrySpanProcessor is based on OpenTelemetry's internal span processor. | ||
| */ | ||
| export interface OpenTelemetrySpanProcessor { |
There was a problem hiding this comment.
Instead of copying the types, I believe it should be possible to add the package with the types as a dev dependency and use import type to import only the type definitions.
There was a problem hiding this comment.
@unflxw not sure what you mean here. If it's a dev dependency how can we use it once the package is shipped?
|
I found an issue with query child spans being reported as root spans and creating new incidents. This is fixed with applying #656, which we should rebase this PR on when merged. |
|
While performing the daily checks some issues were found with this Pull Request.
|
After the last release this file was not updated. Known issue of our publishing set up. Update it now before the next release.
Add a method in the extension and its wrapper to import a span from OpenTelemetry.
When testing locally with the Docker-based OpenTelemetry example, the extension is built for the Linux target. This ensures we don't accidentally commit it.
Do not directly expose the `importOpenTelemetrySpan` function from the client. Instead, expose it from the extension and the wrapper.
To use the types from the OpenTelemetry implementation, depend on
@opentelemetry/api:
npm install --save @opentelemetry/api
Since this patch, the library depends on @opentelemetry/api, to import types for HrTime, Span, SpanAttributes and SpanContext. This patch also uses the SpanProcessor and ReadableSpan types from @opentelemetry/sdk-trace-base, but those are duplicated in src/interfaces/span_processor.ts to avoid dependencies.
If there is no current span in the AppSignal integration for an incoming OpenTelemetry span to be a child of, do not import the OpenTelemetry root span.
This patch is marked as a minor release, as it adds a new span processor which is used to instrument MySQL queries.
- Add `importOpenTelemetrySpan` extension function. - Fix `determine_running_in_container` function for Heroku. - Return seconds and nanoseconds from `span_to_json` function.
The fields returned for the start time have been renamed and the nanoseconds field has been added. Update the tests to match. I haven't implemented the nanoseconds one because it returns negative numbers, something which should not be possible. It appears to be a problem in our Node.js library time conversion.
Fix ESLint warnings that have been present for a couple commits now.
tombruijn
force-pushed
the
ot-span-processor
branch
from
9297098 to
be7fd51
Compare
Explain a bit more what this change is for and how it works. So far no instructions, we'll add this later. [ci skip]
tombruijn
added a commit
that referenced
this pull request
May 11, 2022
Use Typescript's `import type` and `export type` to include the OpenTelemetry types into our package without having to copy the code into our project directly. This was suggested by @unflxw in PR #651 (comment) I'm not sure if this is more or less fragile. If the OpenTelemetry package changes the interface we'd be shipping an older version that doesn't work with older versions anymore. The package from which we import is a dev dependency only, and used during the compile step, and won't ship as a dependency in the final release. I got an issue when using the OpenTelemetry SpanProcessor type for the `onEnd` function. It doesn't have a second argument. The `onStart` also needed another type, from `SpanContext` to `Context`.
tombruijn
added a commit
that referenced
this pull request
May 16, 2022
Use Typescript's `import type` and `export type` to include the OpenTelemetry types into our package without having to copy the code into our project directly. This was suggested by @unflxw in PR #651 (comment) I'm not sure if this is more or less fragile. If the OpenTelemetry package changes the interface we'd be shipping an older version that doesn't work with older versions anymore. The package from which we import is a dev dependency only, and used during the compile step, and won't ship as a dependency in the final release. I got an issue when using the OpenTelemetry SpanProcessor type for the `onEnd` function. It doesn't have a second argument. The `onStart` also needed another type, from `SpanContext` to `Context`.
tombruijn
added a commit
that referenced
this pull request
Jun 8, 2022
In PR #659 we tried to import the OpenTelemetry types at compile time and include them in our own package, but that's not how TypeScript's `import type` turns out to work. This breaks the app for non-OpenTelemetry users as reported in #684. The easiest fix we can ship right now is to add the package as a runtime dependency rather than only a development dependency. Ideally though, we do not ship additional dependencies for thing not all our users use, but we can figure that out after this immediate fix. I had missed this in PR #659, but we've also already included the `@opentelemetry/api` as a dependency since PR #651. Already doing the same this change does for the `@opentelemetry/sdk-trace-base` package.
tombruijn
added a commit
that referenced
this pull request
Jun 8, 2022
In PR #659 we tried to import the OpenTelemetry types at compile time and include them in our own package, but that's not how TypeScript's `import type` turns out to work. This breaks the app for non-OpenTelemetry users as reported in #684. The easiest fix we can ship right now is to add the package as a runtime dependency rather than only a development dependency. Ideally though, we do not ship additional dependencies for thing not all our users use, but we can figure that out after this immediate fix. I had missed this in PR #659, but we've also already included the `@opentelemetry/api` as a dependency since PR #651. Already doing the same this change does for the `@opentelemetry/sdk-trace-base` package.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch adds an OpenTelemetry span processor that takes spans from OpenTelemetry and exports them to the AppSignal agent via its newly added
importOpenTelemetrySpanfunction.Blocked on a new agent release: https://github.com/appsignal/appsignal-agent/pull/756