-
Notifications
You must be signed in to change notification settings - Fork 810
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
Deno Support #2293
Comments
Another thing to be aware of for deno support is the use of XHR. Since deno does not support XHR, when trying to use opentelemetry/metrics via jspm, an error is thrown when creating the request
|
This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days. |
Assigning myself as I'm working on non-main-frame-context support on browsers. |
Fwiw I tried taking a stab at adapting one of the browser console exporter examples for Deno Deploy to see how far I could get, and I was actually able to make it through to the end with a minimum of monkey patching. Here's the Deno Deploy code I used. All of the hacks are just below the imports (the last one was also to get around this Deno Deploy issue)
And here was the console output
And I haven't tried this (yet), but I wonder if a polyfill for XmlHttpRequest for Deno (like this one from deno.land) could be enough to support a normal exporter before standard support arrives. |
I think the main thing that will be missing for correct deno support is a correct context manager, the zone start to fail as soon as you use async/await (in your example you use promises so it works fine). |
(Been trying to read around and unpack your insight after confirming it didn't work with async/await...trying to summarize my understanding below) Am I correct in understanding that browsers today (and maybe for the foreseeable future) don't offer a way to maintain context across native async/await, and so there's no way to update zone.js (or any userland approximation of zones/other constructs that can keep track of timings/contexts across async operations/ticks) to take them into account, so no usage of otel-js (or any similar tooling) will be able to work with them? So the workaround for browsers and Deno for now is to avoid native async/await (either by not using it or transpilation). Is that accurate? (or is there some nuance for browsers I'm maybe missing?) (Fwiw it was this github issue for zone.js that helped crystallize things for me a bit more) |
Exactly, this is a known issue and i believe the general agreement is to have a system added into EMCAScript but no one had the time/resource to push it through. The last one was actually @legendecas (https://github.com/legendecas/proposal-async-context)
Currently yes, but from the few discussions i have seen in Deno there is interested from the core team for diagostics tooling and context management is the most important thing in event loop runtimes (there is more info in a PR that someone from MS pulled up there: denoland/deno#8209) |
It doesn't address the main context management blocker, but in case it's helpful I made a small polyfill for navigator.sendBeacon in Deno, as I noticed that was missing and it was either going to be polyfilling that or XmlHttpRequest in order to get the browser HTTP/JSON exporter to work there. With that I was able to get a full flow through Deno working, but in a very simple case avoiding the native async/await and zone issues discussed above. |
Worth noting that a second attempt at promise hooks might be landing soon: denoland/deno#15475 |
promise hooks did land in deno 1.26 |
@legendecas what is the status of the issue? can we help to get it resolved? |
We have test infrastructures to ensure that the SDK supports environments that are web-compatible (like Deno claims). However, we don't have a test infrastructure to run our tests in Deno to guarantee official support. I've been experimenting with running mocha (the test framework used in the project) in Deno. Due to the incompatibility of ESM resolutions (Deno requires the file extension to be present in the module specifier, like Even though the promise hooks API is exposed in Deno, the fundamental issue is the lack of the API for proper async context propagation: denoland/deno#7010. I'm still pursuing the standardization of async context in TC39, however, it would be a long task. |
Would you be able to expand on this point? I noticed in the Deno PR for promise hooks the author outlined what seemed to be a way to get OTEL-JS working in Deno with the feature in this comment denoland/deno#15475 (comment) (I'll copy their comment below for better visibility) I believe this would be an equivalent implementation for Deno: import { Context, ROOT_CONTEXT } from '@opentelemetry/api';
import { AbstractAsyncHooksContextManager } from './AbstractAsyncHooksContextManager';
interface HookCallbacks {
init: (promise: Promise<unknown>) => void
before: (promise: Promise<unknown>) => void
after: (promise: Promise<unknown>) => void
resolve: (promise: Promise<unknown>) => void
}
const enabledCallbacks = new Set<HookCallbacks>();
Deno.core.setPromiseHooks(
(promise: Promise<unknown>) => {
for (const { init } of enabledCallbacks) {
init(promise);
}
},
(promise: Promise<unknown>) => {
for (const { before } of enabledCallbacks) {
before(promise);
}
},
(promise: Promise<unknown>) => {
for (const { after } of enabledCallbacks) {
after(promise);
}
},
(promise: Promise<unknown>) => {
for (const { resolve } of enabledCallbacks) {
resolve(promise);
}
},
);
export class AsyncHooksContextManager extends AbstractAsyncHooksContextManager {
private _contexts: Map<Promise<unknown>, Context> = new Map();
private _stack: Array<Context | undefined> = [];
private _callbacks: HookCallbacks = {
init: (promise) => {
const context = this._stack[this._stack.length - 1];
if (context !== undefined) {
this._contexts.set(promise, context);
}
},
before: (promise) => {
const context = this._contexts.get(promise);
if (context !== undefined) {
this._enterContext(context);
}
},
after: () => {
this._exitContext();
},
resolve: (promise) => {
this._contexts.delete(promise);
}
}
active(): Context {
return this._stack[this._stack.length - 1] ?? ROOT_CONTEXT;
}
with<A extends unknown[], F extends (...args: A) => ReturnType<F>>(
context: Context,
fn: F,
thisArg?: ThisParameterType<F>,
...args: A
): ReturnType<F> {
this._enterContext(context);
try {
return fn.call(thisArg!, ...args);
} finally {
this._exitContext();
}
}
enable(): this {
enabledCallbacks.add(this._callbacks);
return this;
}
disable(): this {
enabledCallbacks.delete(this._callbacks);
this._contexts.clear();
this._stack = [];
return this;
}
private _enterContext(context: Context) {
this._stack.push(context);
}
private _exitContext() {
this._stack.pop();
}
} As pretty much all async operation in Deno is implemented on top of Promises, tracking them should be enough to keep track of all async contexts. It should work with Deno.serve out of the box, but I didn't test it. Again, it's fairly low level and the interface doesn't help. It would make sense to introduce a more friendly wrapper on the stdlib later on. |
@Grunet it is true that we can implement a context manager like
Just like I mentioned that we don't have a native test infrastructure to run our existing test suites (and adding new tests aligning with our current pattern) with Deno. I'd be happy to know if there is any practice that we can adopt in the project. |
Deno supports |
I tried modifying the basic-tracer-node example in this repository and made 2 adjustments (outside of removing the Jaeger exporting for simplicity, just leaving the console exporting)
Iirc the latter didn't used to work, but it seems to now (!) Here's the example code https://github.com/Grunet/otel-js-deno-experimentation Let me know if I did something wrong with this little experiment! |
There's a little stumbling block with fetch autoinstrumentation I ran into. Documented this in the issue that if resolved will probably fix it |
@legendecas Would it be acceptable to add Deno as a dev dependency for OpenTelemetry-js? If it is, I think Deno support could very well be added to the repository by making packages Deno-first and using |
@arendjr Hmm, I wonder if it would be better to start off with some Deno integration tests first. 🤔 That would be somewhat isolated, and we'd be able to figure out what's working and what is not, and we'll also be able to see if something breaks that already used to work with Deno (I think I tried the metrics SDK with Deno some time ago, but I don't know if it still works. Tests would help in that regard). 🤔 IIUC making packages deno-first would be a huge undertaking (most likely taking months). I think this would be a great topic to bring up at the SIG Meeting (we have one every Wednesday, see https://github.com/open-telemetry/community#implementation-sigs), where people can bring up comments, questions, and concerns. |
You’re right adding Deno integration tests might be a less intrusive first step. From my personal testing, I found most things work with Deno already, but the reliance on NPM creates some barriers, such as Deno Deploy not supporting them. But there are other ways to create native Deno releases without necessarily going Deno-first. One notable exception we’ve found at work is the Prometheus exporter silently failing to start its webserver, so that might be a good starting point to create tests for. IMO an advantage of going Deno-first is that it is relatively easy to support together while maintaining NPM compatibility because it is comes with more tooling out-of-the-box, so it could make for a more developer-friendly experience, so if you would be willing to discuss that at a SIG meeting that would be great. But you’re right it’s not something that would be implemented for all packages in the repo overnight, so it would need a bit of a migration plan. Let me know if you would like more input on that matter. |
According to this Blog post, it seems like support for
That sounds interesting. Would you happen to have some resources on that so that I can do some reading? 🙂 I've seen that it is much easier to do Another issue that I've had back when I was trying things out was that I wasn't able to easily convert our existing tests to deno tests (due to the tooling we're using causing some trouble)
I'd be really in favor of adding tests first for things like this; once we have some initial test setup, we can gradually expand the tests to cover more cases.
I'll put this on the agenda for today's SIG meeting. 🙂
I think we might not have the capacity to work on this at the moment as #4083 (SDK 2.0) will be the next big chunk of work we'll be focusing on and I think adding deno support simultaneously would be a large chunk of work that (I think) would increase the scope beyond what we'd be able to handle. However, as we're planning to go to 2.0, we may be able to make some breaking changes that will make it easier for us to support Deno in the future. So if you have some ideas on breaking changes that would be needed to make our future lives easier, the next few weeks would be the perfect time to bring those up. 🙂 |
That's great! Thanks!
I can think of one big one: Rewrite all your internal imports/exports referencing local files to use export * from './common/attributes'; // Current; BAD
export * from './common/attributes.ts'; // GOOD I see you're also upgrading TypeScript for the 2.0 release, and newer versions have a But there's a catch: The TypeScript compiler insists that you also enable either the Now, the reason all of this is actually a breaking change is that while both Rollup and Now, finally, the reason why I think it would be very useful to switch to using extensions is because, as you mentioned, the importance of integration tests. I think we both agree that it would be good to run integration to test stuff in Deno, and those tests would be running inside the repository. But without those extensions, Deno has no way to directly run your sources, meaning you would always need to generate Deno-compatible packages before you could run any tests on your code. It's possible, but it likely would mean creating another build setup just for the Deno tests alone, and the DX would be far from great. For what it's worth, I could probably find some time to work on the above if your team agrees it would be worthwhile (it would certainly help us to deliver Deno support for our own project; https://autometrics.dev)
Yeah, I think Deno -> Node is much easier indeed, which is why I would still prefer a Deno-first approach. But especially if the extensions are fixed, that is something that could be moved towards gradually over time. Using the current NPM-first approach still could result in specialized Deno builds though, much the same way you can create web builds from NPM packages too. It would require using a bundler, some custom config, and the occassional file with Deno-specific implementations (similar to how you already use
Agreed that would be a good first step. Please let me know if others are open to the idea of using the
Ah, great to hear!
Support for Deno Deploy was the main technical blocker. Beyond that I would just say that, as a Deno user, being instructed to use NPM feels very much like you're being treated as a second-class citizen, and would be a reason to look if there are other libraries that might offer Deno-native support. NPM packages also cannot use other Deno-native libraries. This may not be much of a concern for OpenTelemetry-js (I don't think you have external dependencies anyway, right?), but it's one of the reasons why Deno users may expect NPM packages to integrate less well into their ecosystem. Personally, I also find it highly annoying that NPM packages are distributed as separate |
@arendjr Thank you for the detailed answer. 🙂 We've talked about adding Deno support in the meeting yesterday, and I'll try to summarize what we talked about: Not so good news: The main concern that the SIG (myself included) had was that we don't have the capacity right now to add official support for another runtime. We'd need a significant influx of people who are willing to work on it and who are willing to maintain it in the future. Across this repo and https://github.com/open-telemetry/opentelemetry-js-contrib, the project's scope is huge, and we're spread thin right now with just Node.js and improving browser support on the roadmap. Good(-ish) news: The consensus seemed to be that if there is something that does also benefit us in other areas (we have not explicitly talked about deep-imports, but preventing that is something that I'd consider "beneficial in other areas"). We'd also be open for things that do not require significant effort for us to change, but will make it easier to add deno support in the future. 🙂 (Side note: We've been talking about switching up tooling in previous SIG meetings, using
Would you mind creating a new issue for this (you can just copy over the text about |
Thanks! I've created the requested issue here: #4106 Totallly understandable about not wanting to diverge resources on supporting Deno right now. But just to clarify, if I offer to create some PRs to implement partial/community/experimental Deno support, do you think those might be accepted or would they be rejected out of fear of the maintenance cost? |
Thanks 🙂 Added it to the milestone to discuss, and we'll put the reasoning on the issue.
If we accept the proposal from #4106, I'd assume PRs adding integration tests would also be accepted. For other changes, I think it would largely depend on the type/extent of change. Changing some features to also work on Deno would most likely be fine if the change isn't too Deno-specific (i.e. if a change also makes sense for webworkers, which we already have a testing setup for, I think there will be little pushback in merging the PR). Adding specific files to support Deno might be a tougher sell. |
Are there any plans for supporting Deno runtime? Perhaps it is a bit early to speak of it, however it seems rather possible due to low number of dependencies for opentelemetry implementation. Deno tries to support browser APIs so providing support might be rather easy.
It's already possible to import some opentelemetry-js libs via skypack, however using require (for instance, for
require('lodash.merge')
in opentelemetry/tracing and opentelemetry/metrics, opentelemetry/metrics 2 makes it impossible.Overall seems to be not so many requires:
However, it demands more effort due to testing in Deno runtime and validating not using any Node-specific APIs.
Do you have any plans or thoughts at the moment?
Thanks!
The text was updated successfully, but these errors were encountered: