-
Notifications
You must be signed in to change notification settings - Fork 68
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
NR-355081 Sample app for OTEL entity relationship Docker instrumented with NR infra agent #710
base: main
Are you sure you want to change the base?
Conversation
@@ -0,0 +1,2 @@ | |||
FROM newrelic/infrastructure:latest | |||
ADD newrelic-infra.yml /etc/newrelic-infra.yml |
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.
No need to define a new docker image just for this. Just mount newrleic-infra.yml
using a volume mount.
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.
Following the below docs for Infra agent installation
https://docs.newrelic.com/docs/infrastructure/infrastructure-agent/linux-installation/infra-agent-as-container/
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.
Ooof that's bad advice we're giving our customers.
Now we're stuck answering the question of whether to prioritize consistency with the nr infra agent docs, or the simpler / idiomatic approach for docker. 😅
Can stick with consistency with the nr infra agent docs for the purpose of this PR.
# ...omitted for brevity | ||
# New Relic API key to authenticate the export requests. | ||
# docs: https://docs.newrelic.com/docs/apis/intro-apis/new-relic-api-keys/#license-key | ||
otlp: |
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.
New Relic recommends the http/protobuf version of OTLP.
otlp: | |
otlphttp: |
# New Relic API key to authenticate the export requests. | ||
# docs: https://docs.newrelic.com/docs/apis/intro-apis/new-relic-api-keys/#license-key | ||
otlp: | ||
endpoint: otlp.nr-data.net:4318 |
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.
Other examples configure the OTLP endpoint and API key with env vars to make it easier for NR employees to change which environment they are running the example against. They also defined default values for these env variables in a .env file.
endpoint: otlp.nr-data.net:4318 | |
endpoint: ${NEW_RELIC_OTLP_ENDPOINT} |
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.
handled
``` | ||
|
||
```shell | ||
docker run \ |
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.
Is this the canonical / recommended way to to run the infra agent? If no, let's make sure we align with it.
Also, let's translate this to an entry in the docker-compose.yml
file. No reason to have the user running containers with and without docker compose.
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.
Yes, I am following the below docs for Infra agent installation
https://docs.newrelic.com/docs/infrastructure/infrastructure-agent/linux-installation/infra-agent-as-container/
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.
Can you translate all this to a service entry in ./docker-compose.yaml
?
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 have already tried it, but below command doesn't support in docker-compose.yaml
--cgroupns=host
so that is the reason I have left it as NR docs recommendation
docker run
-d
--name newrelic-infra
--network=host
--cap-add=SYS_PTRACE
--privileged
--pid=host
--cgroupns=host \ # required on cgroup v2
-v "/:/host:ro"
-v "/var/run/docker.sock:/var/run/docker.sock"
YOUR_IMAGE_NAME
otlp: | ||
protocols: | ||
grpc: | ||
zipkin: |
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.
Remove. Apps should send data via OTLP.
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.
Removed
processors: | ||
[ | ||
resourcedetection, | ||
resourcedetection/cloud, | ||
resourcedetection/system, | ||
resource, | ||
batch, | ||
] |
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.
If we're going to use the bracket syntax, let's keep all the entries on one line for readibility. Can also use the array syntax instead:
processors: | |
[ | |
resourcedetection, | |
resourcedetection/cloud, | |
resourcedetection/system, | |
resource, | |
batch, | |
] | |
processors: | |
- resourcedetection | |
- resourcedetection/cloud | |
- resourcedetection/system | |
- resource | |
- batch |
This advice applies to other pipelines as well.
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.
Deleted the file
depends_on: | ||
- collector | ||
|
||
collector: |
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.
Why are you running the collector and the infra agent? Is it because you're relying on the collector setting host.id
for the entity relationship? If yes, can you configure the APM app to include that attribute directly?
As a principle, we want these examples to be minimalist, with everything removed that isn't critical to the functionality being demonstrated.
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.
Changes updated
endpoint: 0.0.0.0:9411 | ||
exporters: | ||
debug: | ||
verbosity: detailed |
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.
Is this necessary?
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.
Deleted
@@ -26,7 +26,7 @@ exporters: | |||
debug: | |||
verbosity: $LOG_EXPORTER_LOG_VERBOSITY | |||
otlphttp: | |||
endpoint: https://otlp.nr-data.net | |||
endpoint: https://otlp.nr-data.net:4317 |
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 not necessary. Our OTLP endpoint listens on port 443, which is the default when the protocol is https. If port was required, we'd want it to be 4318 instead of 4317.
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.
Mistakenly updated, reverted the change
- "4317" | ||
environment: | ||
- AD_SERVICE_PORT=8080 | ||
- OTEL_EXPORTER_OTLP_ENDPOINT=http://otel-collector-container-nr-agent:4318 |
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.
Is the collector actually required for this example? Looking at otel-config.yaml
, I see content which looks like a copy of the collector configuration from the host-monitoring example. But here we're trying to demonstrate monitoring with NR infra agent, so that configuration duplicates data and is a distraction.
If we don't need the collector to do data enrichment to make this relationship work, let's delete it and export directly to New Relic:
- OTEL_EXPORTER_OTLP_ENDPOINT=${NEW_RELIC_OTLP_ENDPOINT}
- OTEL_EXPORTER_OTLP_HEADERS=${NEW_RELIC_API_KEY}
If the collector is in fact needed (I can't tell at a glance), let's get rid of all the configuration which is unrelated.
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 below one worked so I have removed the collector example
- OTEL_EXPORTER_OTLP_ENDPOINT=${NEW_RELIC_OTLP_ENDPOINT}
- OTEL_EXPORTER_OTLP_HEADERS=api-key=${NEW_RELIC_API_KEY}
- NEW_RELIC_OTLP_ENDPOINT | ||
container_name: otel-collector-container-nr-agent | ||
|
||
# Optionally, add Jaeger or another service here |
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.
# Optionally, add Jaeger or another service here |
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.
Removed
This example code created for APM OTEL UI convergence Infra relationships particularly for OTEL entity relationship with docker which is instrumented with NR infra agent.