-
Notifications
You must be signed in to change notification settings - Fork 3
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
Node v18.20.3 nsolid v5.3.0 release #146
Merged
trevnorris
merged 14 commits into
node-v18.x-nsolid-v5.x
from
node-v18.20.3-nsolid-v5.3.0-release
Jun 24, 2024
Merged
Node v18.20.3 nsolid v5.3.0 release #146
trevnorris
merged 14 commits into
node-v18.x-nsolid-v5.x
from
node-v18.20.3-nsolid-v5.3.0-release
Jun 24, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The consumer should add the `grpc++` target (the C++ gRPC library) as a dependency to add gRPC support. A couple of other useful targets have been created, the most relevant for now being `grpc_cpp_plugin` which creates the plugin to be used by `protoc` to create the OTLP gRPC headers with the protobuf definitions. Also, modify `test-strace-openat-openssl` so it takes into account an additional `openat()` syscall. PR-URL: #133 Reviewed-by: Trevor Norris <trev.norris@gmail.com>
While doing it, the updater script `update-opentelemetry-cpp.sh` has been updated so it builds the `grpc_cpp_plugin` to be used by `protoc` to also generate the OTLP gRPC headers with the protobuf definitions. PR-URL: #133 Reviewed-by: Trevor Norris <trev.norris@gmail.com>
To be clear, support hasn't been added just yet to the agent itself, but the needed modifications so it can be easily done: - Added `grpc++` as a direct dependency to `opentelemetry-cpp`. - Added needed source files to our `protobuf` dependency. - Basic modifications to `OTLPAgent` and `OTLPMetrics` so they can support any kind of `OTLP` exporter, some helper code and some actual code that works but remains commented until we decide the best way to add gRPC support (configuration, etc.). PR-URL: #133 Reviewed-by: Trevor Norris <trev.norris@gmail.com>
A new config param `protocol` is added to the `otlpConfig` for the `otlp` exporter. It defaults to `http` but it can also take the `grpc` value to enable basic (insecure) `gRPC` exporting in the agent. For the agent tests, multiple changes were needed: - Added the `@grpc/grpc-js` and `@grpc/proto-loader` packages needed to implement a basic gRPC server. - Add the actual OTLP protobuf files needed to configure the gRPC server. - Create a `OTLPGRPCServer` with the same interface as the current `OTLPServer`. PR-URL: #134 Reviewed-by: Trevor Norris <trev.norris@gmail.com>
Add API to allow retrieving all currently available SharedEnvInst instances. PR-URL: #123 Reviewed-by: Santiago Gimeno <santiago.gimeno@gmail.com>
The user shouldn't have access to start/stop. No need to expose them. Also remove stop() completely. PR-URL: #123 Reviewed-by: Santiago Gimeno <santiago.gimeno@gmail.com>
Allow users to register a callback that will transform the metrics into a std::string that will be written to the endpoint. In order to do this some of the internals needed to be rewritten so that config() could be called from any thread. The internal usage of config() has been renamed since it will always be called from the StatsDAgent thread. PR-URL: #123 Reviewed-by: Santiago Gimeno <santiago.gimeno@gmail.com>
Add a C++ API to collect logs written with the nsolid JS API. PR-UR: #135 Reviewed-by: Santiago Gimeno <santiago.gimeno@gmail.com>
Forgot to check the lint-js output from my last commit. Fix those errors. PR-URL: #139 Reviewed-by: Santiago Gimeno <santiago.gimeno@gmail.com>
The configuration of the OTLPAgent is IMHO a bit clunky. The are 2 configuration params/env variables: `NSOLID_OTLP` and `NSOLID_OTLP_CONFIG`. `NSOLID_OTLP` can take multiple values depending on the backend we're planning to send the data to: `datadog`, `dynatrace`, `newrelic` and `otlp`. `NSOLID_OTLP_CONFIG` takes a JSON object with the config which has different format depending on the `NSOLID_OTLP` value. For the case of `otlp` the format of the config is something like this: ``` { url: 'http://endpoint:port', protocol: 'http' // or 'grpc' } ``` This only gets us so far: what happens if I want to add a key in a specific heep header? what if I want to send metrics to and endpoint and traces to a different one? I could keep adding fields to the config object but, do we want this? Luckily the OTLP exporter spec defines a series of standard env variables which allows for very granular configuration of the exporter (https://opentelemetry.io/docs/specs/otel/protocol/exporter/). These env variables are supported by default by `opentelemetry-cpp` library we're using. To enable this, a very slight modification of `NSOLID_OTLP` and `NSOLID_OTLP_CONFIG` is needed. In the event we want the agent to use these variables we'll need to configure only `NSOLID_OTLP=otlp` and not configure `NSOLID_OTLP_CONFIG`, which was required until now. In case we use this new configuration and no OTLP_* env variable is used, the agent will be launched configure with the default values defined in https://opentelemetry.io/docs/specs/otel/protocol/exporter/ So for example, now if we want to send metrics using grpc to endpoint1 and traces using http to endpoint2 I would do it this way: ``` OTEL_EXPORTER_OTLP_METRICS_PROTOCOL=grpc OTEL_EXPORTER_OTLP_METRICS_ENDPOINT=http://endpoint1:${port}/v1/metrics OTEL_EXPORTER_OTLP_TRACES_PROTOCOL=http/protobuf # this could be omitted as it's the default OTEL_EXPORTER_OTLP_TRACES_ENDPOINT=http://endpoint2:${port}/v1/metrics ./nsolid ``` PR-URL: #138 Reviewed-by: Trevor Norris <trev.norris@gmail.com>
We want the ZmqAgent to collect data (traces and/or metrics) even if the ZMQ channels aren't fully up. This wasn't happening and we were losing data because the tracing and metrics configuration options were not handled until the ZmqCommandHandle wasn't ready. Ideally we should be able to collect traces even on initial loop iteration without the need of calling `nsolid.start()`. This should go into a follow-up PR though. PR-URL: #140 Reviewed-by: Trevor Norris <trev.norris@gmail.com>
Avoid calling `child.send('exit')` multiple times as it might trigger an `EPIPE` error. PR-URL: #143 Reviewed-by: Trevor Norris <trev.norris@gmail.com>
Running the test with the following: $ ./tools/test.py -J --repeat=100 test/addons/nsolid-statsdagent/nsolid-statsdagent.js caused the test to fail in various ways. The fix takes into account that when the test is slowed down under load it's possible for the StatsDAgent instance to be ready before it can be checked in JS, and that the data might have been placed into a single call to 'data'. Fixes: #142 PR-URL: #144 Reviewed-by: Santiago Gimeno <santiago.gimeno@gmail.com>
santigimeno
approved these changes
Jun 24, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RSLGTM
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.