search

amzn / hawktracer

HawkTracer is a highly portable, low-overhead, configurable profiling tool built in Amazon Video for getting performance metrics from low-end devices.
MIT License
133 stars 31 forks source link

Add node.js bindings #67

Open beila opened 4 years ago

beila commented 4 years ago

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

Still need to do:

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

beila commented 4 years ago

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 commented 4 years ago

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 notifications@github.com wrote:

@loganek commented on this pull request.

In bindings/nodejs/hawktracer_client_nodejs.cpp https://github.com/amzn/hawktracer/pull/67#discussion_r429650776:

  • 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 https://github.com/amzn/hawktracer/pull/67#discussion_r429650776, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABDE5UJPTM7LLHY5EWKX2I3RTFACBANCNFSM4MCJPIVA .