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

Implement PerformanceObserver #4708

Closed
Tracked by #442
paymog opened this issue Sep 9, 2023 · 20 comments
Closed
Tracked by #442

Implement PerformanceObserver #4708

paymog opened this issue Sep 9, 2023 · 20 comments
Labels
enhancement New feature or request web-api Something that relates to a standard Web API

Comments

@paymog
Copy link

paymog commented Sep 9, 2023

What is the problem this feature would solve?

It would be great if Bun could implement the PerformanceObserver which seems to be required for using the prom-client package.

EDIT: turns out this is only required when collectDefaultMetrics is called on the client.

 ❮❮❮ bun src/index.ts
 8 | }, $;
 9 |
10 | class NotImplementedError extends Error {
11 |   code;
12 |
13 |   constructor(feature, issue) {
                             ^
NotImplementedError: PerformanceObserver is not yet implemented in Bun.
 code: "ERR_NOT_IMPLEMENTED"

      at new NotImplementedError (internal:shared:13:26)
      at internal:shared:2:68
      at new PerformanceObserver (node:perf_hooks:17:26)
      at /Users/paymahn/code/goldsky/rpc-node-proxy/node_modules/prom-client/lib/metrics/gc.js:44:13
      at collectDefaultMetrics (/Users/paymahn/code/goldsky/rpc-node-proxy/node_modules/prom-client/lib/defaultMetrics.js:47:2)
      at /Users/paymahn/code/goldsky/rpc-node-proxy/src/metrics/prometheus.ts:10:0
      at processTicksAndRejections (:1:2602)

What is the feature you are proposing to solve the problem?

Implementation of the PerformanceObserver which is currently not implemented

What alternatives have you considered?

None yet, I'm very new to the ecosystem.

@paymog paymog added the enhancement New feature or request label Sep 9, 2023
@kristofgazso
Copy link

This is currently the only blocker for us to start using Bun 💯💯💯

@vladislav-sidorovich
Copy link

I would like to support @kristofgazso . For our company it's also the single blocker.

@vicwolk
Copy link

vicwolk commented Sep 12, 2023

This is our only blocker as well.

@agjs
Copy link

agjs commented Sep 12, 2023

Only blocker as well. Getting the error when trying to run our Fastify server, due to the fact that we are using fastify-metrics, that also uses prom-client.

@MarkCupitt
Copy link

Yep, us too, we would have to rewrite a bunch of container apps or remove prom-client

@agjs
Copy link

agjs commented Sep 14, 2023

Hey @Jarred-Sumner. I'd like to maybe give Zig a shot and try implementing this feature in Bun. Keep in mind, I never wrote Zig, nor I ever contributed to this repository, but it would be great to have an excuse.

Beside setting up the Bun development environment, could you give me a guideline or two where to start looking.

Thanks

@Tjerk-Haaye-Henricus
Copy link

For us it would also be really important :D We would like to see Bun Performance in our reports 😎

@dipbd1
Copy link

dipbd1 commented Oct 6, 2023

This is a blocker for my project too. I would love to contribute.

@o-az
Copy link
Contributor

o-az commented Oct 6, 2023

This is a currently the only blocker for https://github.com/0xOlias/ponder too.

Ponder uses prom-client which requires this API

@marziply
Copy link

Bun is a prime candidate for replacing Node for microservices I am working on. By commenting out collectDefaultMetrics, these services work perfectly. This issue is a blocker on any plans for migrating to Bun since Prometheus is not a tool I can remove.

@stavalfi
Copy link

Also a blocker for us. Can't remove Prometheus.

@maximilianschmid
Copy link

Last blocker for Planfred to run on bun

@Electroid Electroid added the web-api Something that relates to a standard Web API label Oct 27, 2023
@jnovax
Copy link

jnovax commented Nov 4, 2023

Love to see it soon.

@Jarred-Sumner
Copy link
Collaborator

The good news is we can almost entirely re-use the implementation from Safari for this API.

I think this would mostly look like:

  1. Copy these files:
  1. Build the browser version of WebKit locally, and copy the coreesponding generated JS binding files
  2. Fix any of the build errors that come up along the way
  3. Add mark, measure, etc to the performance object
  4. Add PerformanceObserver globally
  5. Do a pass through the related APIs and see if there are any other globals that will need to be exposed

I don't love this approach because it would be nice to emit various events from zig. I also don't love how many memory allocations are involved in WebKit's implementation. performance.mark and performance.measure needs to be pretty cheap. But this approach is straightforward to ship (and would be faster than an equivalent JS approach)

@agjs
Copy link

agjs commented Dec 21, 2023

Any movement on this?

@MarkCupitt
Copy link

MarkCupitt commented Dec 21, 2023 via email

@gchicoye
Copy link

Hi, any news on that? Thanks a lot!!!

@Jarred-Sumner
Copy link
Collaborator

This will ship in Bun v1.0.22, thanks to @gvilums

@mishraomp
Copy link

mishraomp commented Apr 1, 2024

can this issue be reopened @Jarred-Sumner , I tried to run bun , but its kicking off this error both in windows and docker environments


16 |
17 | class NotImplementedError extends Error {
18 |   code;
19 |   constructor(feature, issue) {
                               ^
NotImplementedError: perf_hooks.monitorEventLoopDelay is not yet implemented in Bun.
 code: "ERR_NOT_IMPLEMENTED"

      at new NotImplementedError (internal:shared:19:27)
      at internal:shared:2:69
      at monitorEventLoopDelay (node:perf_hooks:123:47)
      at /usr/src/app/node_modules/prom-client/lib/metrics/eventLoopLag.js:47:21
      at collectDefaultMetrics (/usr/src/app/node_modules/prom-client/lib/defaultMetrics.js:47:3)
      at /usr/src/app/prom.js:34:1
      at /usr/src/app/metrics.controller.js:17:7
      at /usr/src/app/app.module.js:17:7`

here is the link to the source code which I wanted to run Existing nest/express API with Node
https://github.com/bcgov/nr-omrr-transparency/tree/main/backend

@rhinodavid
Copy link

can this issue be reopened @Jarred-Sumner , I tried to run bun , but its kicking off this error both in windows and docker environments
Looks like it's here: #9432

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request web-api Something that relates to a standard Web API
Projects
None yet
Development

No branches or pull requests