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

API re-design #52

Merged
merged 22 commits into from
Sep 16, 2020
Merged

API re-design #52

merged 22 commits into from
Sep 16, 2020

Conversation

dnlup
Copy link
Owner

@dnlup dnlup commented Aug 13, 2020

To add some new features, deal with the stateful nature of the eventLoopDelay Histogram and give the user more ways to extract data from the collected metrics, I decided to re-design the whole API of the module.

TODO:

  • change the event name from data to sample
  • don't pass metrics as an event argument
  • attach metrics as objects with independent APIs to the Doc instance
  • add autostart and unref options
  • fix types
  • fix tests
  • add docs

@dnlup dnlup changed the title Major API re-design API re-design Aug 13, 2020
@dnlup dnlup self-assigned this Aug 13, 2020
@dnlup dnlup added the breaking change This will cause a breaking change label Aug 13, 2020
@dnlup dnlup added this to the Version 2 milestone Aug 13, 2020
This was linked to issues Aug 13, 2020
@dnlup dnlup removed a link to an issue Aug 14, 2020
@dnlup dnlup mentioned this pull request Aug 14, 2020
Start the work on redesign doc metrics to carry state.
Attach cpu metric on doc instance.
Change the `data` event to `sample`.
Don't pass arguments to the event.

BREAKING CHANGE: the cpu metric is not exposed anymore to the
event handler, but it is attached to the Doc instance. The event name is
changed from `data` to `sample`.
Attach metric to Doc instance to expose its API to the user, as done
with the cpu.

BREAKING CHANGE: the eventLoopDelay metric is not exposed anymore to the
event handler, but it is attached to the Doc instance.
Expose the metric directly from the Doc instance.

BREAKING CHANGE: the metric is not exposed to the event handler anymore,
but it is attached directly to the Doc instance.
Attach memory and activeHandles to Doc instance. Add config options to
manually start metrics collection and ref the underlying timer.
Export types without using a namespace.

BREAKING CHANGE: types are not accessible using `Doc.` notation.
`DocInstance` has been renamed to `Doc` and declared as a class.
@dnlup
Copy link
Owner Author

dnlup commented Aug 20, 2020

@acheronfail I changed many things in the types definitions, not sure if I messed them up. I removed the namespace and exported both a default factory function and the Doc class with its types. I think this would allow me to export a Stream object in the future. Could you check that part, if you got the time (there's no hurry, of course)?

Copy link
Contributor

@acheronfail acheronfail left a comment

Choose a reason for hiding this comment

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

The new changes look great! Very excited.

Just a few comments here and there on the types, but mostly good. 🙂

index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@dnlup
Copy link
Owner Author

dnlup commented Sep 4, 2020

@acheronfail I should have addressed your suggestions. Thank you. Could you take another look?

Expose a `compute` method to allow the user to compute other values from
the event loop delay histogram.
Rename the Doc class to Sampler.

BREAKING CHANGE: the exported class is named Sampler and not Doc anymore
Align tests with the last refactor.
@acheronfail
Copy link
Contributor

All looks good to me mate 👍 Very excited about these changes. 🚀

Align symbols names of the Sampler. Change descriptions to follow
Node.js standard.
See https://github.com/nodejs/node/blob/master/doc/guides/using-symbols.md
@dnlup dnlup force-pushed the api_redesign branch 2 times, most recently from acdbee1 to decca27 Compare September 15, 2020 06:55
Write the readme from the ground up to reflect the API changes.
@dnlup dnlup marked this pull request as ready for review September 16, 2020 05:34
@dnlup dnlup changed the base branch from gc_improvements to next September 16, 2020 05:35
@dnlup dnlup mentioned this pull request Sep 16, 2020
3 tasks
@dnlup dnlup merged commit 4fae09f into next Sep 16, 2020
@dnlup dnlup deleted the api_redesign branch September 16, 2020 06:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change This will cause a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove raw field in data event object
2 participants