feat(new sink): New Relic sink for Events, Metrics and Logs #9764
Conversation
…ctor into feature/new_relic_sink
|
All changes suggested by the reviewer have been applied (except NewRelicSinkError, see the conversation for details). |
src/sinks/new_relic/config.rs
Outdated
|
|
||
| Ok(( | ||
| super::VectorSink::Stream(Box::new(sink)), | ||
| future::ok(()).boxed(), |
There was a problem hiding this comment.
Can you add a healthcheck? You can look at the existing new relic sink for an example. This is useful to check that the config is correct, and Vector can talk to the API.
There was a problem hiding this comment.
Regarding health checks, the New Relic APIs don't document a way to check the status using the API itself (they have a status page, but not something you could request with an API), there is no documented GET, HEAD, or any other method besides the POST used to send the actual data. Although using the POST method with an empty object to check the availability is probably possible, is highly discouraged, because it could contribute to reach the API request limits, degrading the service performance.
https://docs.newrelic.com/docs/data-apis/ingest-apis/introduction-event-api/
https://docs.newrelic.com/docs/data-apis/ingest-apis/metric-api/introduction-metric-api/
https://docs.newrelic.com/docs/logs/log-api/introduction-log-api/
EDITED: Check #9764 (comment)
There was a problem hiding this comment.
The healthcheck is only ran a single time when Vector starts up, so I wouldn't worry about API request limits. (And at least for the Events API, you can send 100000 / minute), so if that is your only concern I'd suggest still adding some kind of health check.
The current new_relic_logs sends an empty HEAD request and checks the status code response, but if an empty POST also works, that could be reasonable too.
There was a problem hiding this comment.
Sounds fair. Only one question, what if the health check fails? As you said the new_relic_log sink uses a HEAD call, but this method is not documented in the official New Relic API descriptions, so, one would expect that this could suddenly change or even disappear. What would happen in such a case? The sink would continue working or Vector will prevent it from running?
There was a problem hiding this comment.
If the healthcheck fails that specific sink won't run. However, if something breaks the healthcheck it can easily be disabled (eg: https://vector.dev/docs/reference/configuration/sinks/new_relic_logs/#healthcheck.enabled)
There was a problem hiding this comment.
There was a problem hiding this comment.
I think there is a bit of misunderstanding here on the purpose of the healthcheck. This is something that is ran once when Vector starts and is mostly there to verify that the configuration of Vector is correct and it's able to talk to the service. So it should be able to detect issues such as invalid credentials, or invalid network configuration (can't reach the endpoint). Checking the status endpoint does verify part of this, but leaves out checking the credentials. Ideally this would check an endpoint that is actually used by the sink (such as sending an empty POST to upload events, if that is a valid request) so that it is testing everything.
We are working on updating the component spec documentation to further clarify the health checks to make this more clear in the future.
There was a problem hiding this comment.
For more info about health checks, see https://github.com/vectordotdev/vector/blob/master/docs/DEVELOPING.md#sink-healthchecks
|
Most of the requested changes are ready. See open conversations for details. |
src/sinks/new_relic/config.rs
Outdated
|
|
||
| Ok(( | ||
| super::VectorSink::Stream(Box::new(sink)), | ||
| future::ok(()).boxed(), |
There was a problem hiding this comment.
The healthcheck is only ran a single time when Vector starts up, so I wouldn't worry about API request limits. (And at least for the Events API, you can send 100000 / minute), so if that is your only concern I'd suggest still adding some kind of health check.
The current new_relic_logs sends an empty HEAD request and checks the status code response, but if an empty POST also works, that could be reasonable too.
This prompted by some [discussion on the New Relic sink](#9764 (comment)) Signed-off-by: Jesse Szwedko <jesse@szwedko.me>
…10431) * docs(internal docs): Add note about health checks to component spec This prompted by some [discussion on the New Relic sink](#9764 (comment)) Signed-off-by: Jesse Szwedko <jesse@szwedko.me>
|
Thanks @asllop for your contribution! |
Description
A sink for the main New Relic APIs: Events, Metrics, and Logs. The specific API used is specified in the config TOML. This PR includes the sink code, tests, and documentation.
Features
Deprecation
new_relic_logsink, but it surpasses its functionalities: it adds support for NR Events and Metrics, and uses recommended JSON format for grouping samples supported by NR APIs. Also applies some integrity rules to events.