-
Notifications
You must be signed in to change notification settings - Fork 223
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
[NA] Remove New Relic dependency #254
[NA] Remove New Relic dependency #254
Conversation
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.
The change looks generally good to me, just a minor recommendation about the configuration.
Please fill up the description template of this PR. I'm missing information, specially about:
- How was this tested?
- Any test follow-up actions after deploying?
- What infrastructure do we need to deploy in order for this to work? Do we require deploying the OTEL agent priorly.
- Does this work directly with New Relic? Or are there any follow-up actions?
- What about Prometheus/Grafana?
Hi @andrescrz
|
b0baad7
to
b502e45
Compare
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.
LGTM, please make sure this is completely enabled after your testing in staging and before going to production.
We don't want to miss the current instrumentation if any problem arises in production.
789eb74
5b9866b
to
789eb74
Compare
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.
I think it's better that you move ahead and merge this soon, no need to wait. I changed my mind as we can deploy specific tags.
<dependency> | ||
<groupId>io.opentelemetry.instrumentation</groupId> | ||
<artifactId>opentelemetry-r2dbc-1.0</artifactId> | ||
<version>${opentelmetry.version}-alpha</version> |
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.
I found mutiple opentelemetry libraries for R2DBC. This seems to be an early one in alpha state. Why this choice over others?
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.
This is the agent instrumentation, and all modules are in alpha: https://mvnrepository.com/artifact/io.opentelemetry.instrumentation/opentelemetry-r2dbc-1.0. Here is the definition of alpha:
https://github.com/open-telemetry/opentelemetry-java/blob/main/VERSIONING.md#stable-vs-alpha. Basically, they mention such modules don't have backward compatibility guarantees and a few other problems that may require changes from version to version, but that doesn't mean they are not mature.
295472c
Fixed all issues by setting some envious. I will merge it on Monday |
Details
In order to make it work, we only need to set the disable envvar to false and the the following variables:
Issues
Resolves #
Testing
The integration was tested locally using a custom API key from our OTEL provider.
Documentation