Add node.js bindings #67

beila · 2020-04-06T13:56:15Z

Description of changes:
Node.js apps can access HawkTracer data through 'hawk-tracer-client' package.

Still need to do:

fix stalling after test run
make it available as a npm dependency directly from git repo
add types for typescript
~~[ ] figure out deployment~~
remove unneeded line (Add node.js bindings #67 (comment))
move TS-related files to bindings/nodejs (Add node.js bindings #67 (comment), Add node.js bindings #67 (comment))
catch exception in the example (Add node.js bindings #67 (comment))
check the klass from klass_id (Add node.js bindings #67 (comment), Add node.js bindings #67 (comment))
set label only for HT_CallstackIntEvent (Add node.js bindings #67 (comment))
check if klass_name exists (Add node.js bindings #67 (comment))

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Still need to do: - buffering events between reader thread and js thread - clean up buffered events - figure out deployment

loganek

@beila Thanks a lot for the contribution! Unfortunately I didn't have chance to review the whole code yet, but I'm posting some comments already so you can have a look and respond.

package.json

bindings/nodejs/CMakeLists.txt

loganek · 2020-04-08T20:18:09Z

bindings/nodejs/CMakeLists.txt

+            hawktracer_client_nodejs.hpp
+            client_context.cpp
+            client_context.hpp)
+    set_target_properties(nodejs_bindings_client PROPERTIES PREFIX "" OUTPUT_NAME hawk_tracer_client SUFFIX .node)


Suggested change

set_target_properties(nodejs_bindings_client PROPERTIES PREFIX "" OUTPUT_NAME hawk_tracer_client SUFFIX .node)

set_target_properties(nodejs_bindings_client PROPERTIES PREFIX "" OUTPUT_NAME hawktracer_client SUFFIX .node)

The project name is HawkTracer in README.md. Isn't it two words?

bindings/nodejs/client_context.hpp

bindings/nodejs/hawktracer_client_nodejs.hpp

- Invert ENABLE_CLIENT condition - Prefix private member functions with underscore - Use make_unique - Remove empty initializer list - Fix space around asterisk and ampersand - Use just vector for _buffer

beila · 2020-04-29T14:56:45Z

examples/nodejs-client/package.json

+  "main": "index.js",
+  "license": "MIT",
+  "dependencies": {
+    "@hawktracer/client": "https://github.com/amzn/hawktracer.git"


Until this PR is merged, this can be tested with "https://github.com/beila/hawktracer.git#node_js_support3".

beila · 2020-04-29T14:59:38Z

package.json

+    "@types/bindings": "^1.3.0",
+    "bindings": "^1.3.0",
+    "cmake-js": "^6.1.0",
+    "node-addon-api": "https://github.com/nodejs/node-addon-api.git#7f56a78"


I used this commit because it fixes nodejs/node-addon-api#660 but it's not included in any released version yet.

loganek

I think overall we should be focusing on 2 things that are separate:

provide low-level node.js bindings which expose raw events as they are and expose API for querying the registry
provide high-level helper classes that help users e.g. converting integer labels into strings etc.

At the moment those two functionalities are tightly coupled and the code assumes that user always want to use our helper methods. I think it'd be valuable to separate those two, so the user can access low-level API from nodejs if needed.

Having said that, I think the contribution so far is enormous and extremely valuable. Having current changes we can already build simple clients with nodejs. I think we could work a bit more on having the API more flexible (i.e. exposing low-level APIs) so even more users could benefit from that.

loganek · 2020-05-10T14:28:02Z

client/CMakeLists.txt


 add_executable(hawktracer-converter main.cpp)

 target_link_libraries(hawktracer_client hawktracer_parser hawktracer_client_utils)
 target_include_directories(hawktracer_client PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/..)
+target_include_directories(hawktracer_client INTERFACE .)


Why do we need this? Having target_include_directories(hawktracer_client PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/..) should be enough.

Yes I only needed when I accessed this from another library, which was reverted before pushing. I'll remove it.

loganek · 2020-05-10T14:48:13Z

tsconfig.json

@@ -0,0 +1,67 @@
+{


could this file be placed in bindings/nodejs?

It should be. I'll try.

loganek · 2020-05-10T14:49:09Z

package.json

@@ -0,0 +1,28 @@
+{


Could this file be in bindings/nodejs?

loganek · 2020-05-10T14:50:02Z

package.json

+    "install": "cmake-js compile --target nodejs_bindings_client --CDENABLE_CLIENT=YES --CDENABLE_NODEJS_BINDINGS=YES --CDBUILD_STATIC_LIB=YES"
+  },
+  "dependencies": {
+    "@types/bindings": "^1.3.0",


Isn't it dev dependency?

At the moment, the typescript files are built at the time of installation, not deployment. So this will have to be installed for the dependent packages too.

loganek · 2020-05-10T14:50:10Z

package.json

+  "dependencies": {
+    "@types/bindings": "^1.3.0",
+    "bindings": "^1.3.0",
+    "cmake-js": "^6.1.0",


isn't it dev dependency?

At the moment, the c++ files are built at the time of installation, not deployment. So this will have to be installed for the dependent packages too.

loganek · 2020-05-10T15:08:25Z

index.ts

+
+function isEventKlassInfoEvent(event: BareEvent): event is EventKlassInfoEvent {
+    const e = event as EventKlassInfoEvent;
+    return e.info_klass_id !== undefined && e.event_klass_name !== undefined;


I don't think this comparison is strong enough. Somebody might define a custom event with those two fields, which is not eventklassinfo event. We should be rather using klass_id for comparison to make sure this is the right class.

I tried to avoid depending on global state, since it's a special type of functions called type guards.
While it may limit the usability a bit, I'll change it to check for klass_id and make it normal function.

loganek · 2020-05-10T15:09:02Z

index.ts

+
+export function isCallstackEvent(event: CallstackEvent): event is CallstackEvent {
+    const e = event as CallstackEvent;
+    return e.duration !== undefined && e.thread_id !== undefined;


I think we should be rather looking at klass_id (or klass_name) to make sure that the event is callstackevent

I'll do the same as isEventKlassInfoEvent.

loganek · 2020-05-10T15:09:54Z

index.ts

+        const events = bareEvents as Event[];
+        events.forEach(e => {
+            // @ts-ignore
+            e.klass_name = this._klass_names[e.klass_id];


instead of keeping duplicated registry in javascript, could we copy rely on eventklassregistry from native code?

loganek · 2020-05-10T15:17:44Z

bindings/nodejs/hawktracer_client_nodejs.cpp

+        o.Set(it.first, _convert_field_value(env, it.second));
+    }
+    if (!event.get_label_string().empty()) {
+        o.Set("label", String::New(env, event.get_label_string()));


I'm not sure if that's the right approach; the label might be an actual field in the event, and we probably shouldn't be replacing it. It's done in the native library (client_utils) because client_utils was supposed to be used only for the end-user applications. Since we're building a nodejs bindings (but not end-user application), I'm not sure we should be manipulating fields, as this is something the customer might not want.

label is defined in core_events.h, not by the user.
How about doing this only for HT_CallstackIntEvent where it's predefined?
When the map files are provided, the user clearly wants to see the string labels, not the int labels.

The whole mapping thing is more like a client feature, not a parser feature. I'd recommend to provide bindings only for parser first, and we might extend it to client if needed.

loganek · 2020-05-10T15:18:42Z

index.ts

+        const events = bareEvents as Event[];
+        events.forEach(e => {
+            // @ts-ignore
+            e.klass_name = this._klass_names[e.klass_id];


What will happen if event already has klass_name field? Will that be overwritten?

It will be overwritten.
I think it's a trade-off between data-safety and usability.
As I mentioned in another comment, for javascript/typescript users, it feels unnatural and uncomfortable to get the type information from somewhere else other than the object itself.
I considered having a generic function that returns the event only when the klass matches, but it would take some time to write, at least for me.
Plus, the term "klass" is being used several times in predefined events in hawktracer. I don't see it being used frequently here.
So my decision came from that it'd be really useful, and that in only rare cases user data would be overwritten.
How about checking whether the event already has "klass_name" field and avoid overwriting it, and just log a warning in such a case?

I think we might consider making the C++ layer as thin as possible, expose C++ interfaces almost 'as they are' to JS, and then on top of the low-level JS binding layer, build more user-friendly JS layer, so in case somebody needs access to low-level bindings, can have it.

beila · 2020-05-11T16:40:48Z

I think overall we should be focusing on 2 things that are separate:

provide low-level node.js bindings which expose raw events as they are and expose API for querying the registry

provide high-level helper classes that help users e.g. converting integer labels into strings etc.

At the moment those two functionalities are tightly coupled and the code assumes that user always want to use our helper methods. I think it'd be valuable to separate those two, so the user can access low-level API from nodejs if needed.

Having said that, I think the contribution so far is enormous and extremely valuable. Having current changes we can already build simple clients with nodejs. I think we could work a bit more on having the API more flexible (i.e. exposing low-level APIs) so even more users could benefit from that.

I don't think having raw events APIs would actually help users. JavaScript/TypeScript users are already accustomed to checking the existence of properties. Checking if a particular property exists is much more familiar and convenient than getting the type information from somewhere else than the object itself.
Having said that, even though we decide to provide the raw events APIs, I think it'd better be done in another PR.

beila · 2020-05-30T09:37:43Z

As a matter of fact, we need the source mapping feature right now. I don't see any customer facing profiling tool for callstack events to be practically useful without source mapping. The whole idea of having C++ layer below javascript bindings was to reuse existing/stable code. I was able to add this mapping feature using the existing mapping code in short time. If I have to implement source mapping on top of low-level bindings, I'll have to rewrite that part. To be honest, I don't think we can do that at this point. If providing low-level APIs is important, what do you think about providing both mapping/non-mapping APIs from the same implementation? In effect, it would be something like disabling source mapping for low-level non-mapping API. If we decide on that, we'll have to decide how to enable/disable that feature. We could either use an option, or another set of separate APIs.

…

On Sun, 24 May 2020 at 17:00, Marcin Kolny ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In bindings/nodejs/hawktracer_client_nodejs.cpp <#67 (comment)>: > + return String::New(env, value.value.f_STRING); + case parser::FieldTypeId::POINTER: + return String::New(env, "(pointer)"); + default: + assert(0); + } +} + +Object Client::_convert_event(const class Env& env, const LabeledEvent& event) +{ + auto o = Object::New(env); + for (const auto& it: event.get_values()) { + o.Set(it.first, _convert_field_value(env, it.second)); + } + if (!event.get_label_string().empty()) { + o.Set("label", String::New(env, event.get_label_string())); The whole mapping thing is more like a client feature, not a parser feature. I'd recommend to provide bindings only for parser first, and we might extend it to client if needed. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#67 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABDE5UJPTM7LLHY5EWKX2I3RTFACBANCNFSM4MCJPIVA> .

Add node.js bindings

76abcc6

Still need to do: - buffering events between reader thread and js thread - clean up buffered events - figure out deployment

loganek reviewed Apr 8, 2020

View reviewed changes

beila and others added 5 commits April 15, 2020 12:17

Prevent _context_holder from being deleted twice

7d3196c

Fix stalling after test run

d52ac2b

Merge branch 'master' into node_js_support3

9ee2096

Remove unnecessary unique_ptr and change style

4f07400

- Invert ENABLE_CLIENT condition - Prefix private member functions with underscore - Use make_unique - Remove empty initializer list - Fix space around asterisk and ampersand - Use just vector for _buffer

Add TypeScript wrapper

f34e7be

beila commented Apr 29, 2020

View reviewed changes

loganek reviewed May 10, 2020

View reviewed changes

Ref Client when starting

1a5e801

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add node.js bindings #67

Add node.js bindings #67

beila commented Apr 6, 2020 •

edited by loganek

Loading

loganek left a comment

loganek Apr 8, 2020

beila Apr 17, 2020

beila Apr 29, 2020 •

edited

Loading

beila Apr 29, 2020

loganek left a comment

loganek May 10, 2020

beila May 11, 2020

loganek May 10, 2020

beila May 11, 2020

loganek May 10, 2020

loganek May 10, 2020

beila May 11, 2020

loganek May 10, 2020

beila May 11, 2020

loganek May 10, 2020

beila May 11, 2020

loganek May 10, 2020

beila May 11, 2020

loganek May 10, 2020

loganek May 10, 2020

beila May 11, 2020

loganek May 24, 2020

loganek May 10, 2020

beila May 11, 2020

loganek May 24, 2020

beila commented May 11, 2020

beila commented May 30, 2020 via email

	set_target_properties(nodejs_bindings_client PROPERTIES PREFIX "" OUTPUT_NAME hawk_tracer_client SUFFIX .node)
	set_target_properties(nodejs_bindings_client PROPERTIES PREFIX "" OUTPUT_NAME hawktracer_client SUFFIX .node)

Add node.js bindings #67

Are you sure you want to change the base?

Add node.js bindings #67

Conversation

beila commented Apr 6, 2020 • edited by loganek Loading

loganek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beila Apr 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loganek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beila commented May 11, 2020

beila commented May 30, 2020 via email

beila commented Apr 6, 2020 •

edited by loganek

Loading

beila Apr 29, 2020 •

edited

Loading