Skip to content
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

Optimize and align host monitoring config with newrelic-opentelemetry-examples #115

Merged

Conversation

jack-berg
Copy link
Contributor

Optimizations:

  • Reduce collection interval to default 60s
  • Disable process metrics to align with NRI defaults
  • Add cumulativetodelta processor to align with New Relic OTLP preferences
  • Drop metric description and unit

Separate cleanup including:

  • Use otlphttp exporter instead of otlp to align with New Relic OTLP preferences
  • Split out dedicated pipelines for host metrics and logs

Corresponding synchronization PR in newrelic-opentelemetry-examples: newrelic/newrelic-opentelemetry-examples#636

cc @alanwest

alanwest
alanwest previously approved these changes Jul 12, 2024
@rubenruizdegauna
Copy link
Contributor

I used this config and I saw some errors:

Exporting failed. Will retry the request after interval.        {"kind": "exporter", "data_type": "metrics", "name": "otlphttp", "error": "failed to make an HTTP request: Post \"staging-otlp.nr-data.net:4317/v1/metrics\": unsupported protocol scheme \"staging-otlp.nr-data.net\"", "interval": "5.150597003s"}

and metrics are not being sent.

cc @josemore to be aware of this change

@josemore
Copy link

some context / notes:

  • "Disable process metrics to align with NRI defaults": the guided install experience for the infra agent (main entry point to install the agent) does enable process monitoring by default and it's surfaced in the host entity detail view. I'm OK disabling this as it's too verbose (high-cost vs value) via OTel at the moment until we have a better solution (might require alignment with Infra experience stakeholders).
  • "interval to default 60s": I understand we defined same interval as default for the same metrics in the infra agent (20s), similarly as above, ok to change this to offer better defaults to OTel customers.
  • As this is entering Preview soon with related new capabilities, we need to ensure the config is valid and we don't break the Infra experiences.

@jack-berg
Copy link
Contributor Author

"Disable process metrics to align with NRI defaults": the guided install experience for the infra agent (main entry point to install the agent) does enable process monitoring by default and it's surfaced in the host entity detail view. I'm OK disabling this as it's too verbose (high-cost vs value) via OTel at the moment until we have a better solution (might require alignment with Infra experience stakeholders).

My understanding is that the answer of whether process metrics are enabled by default or not varies based on the install workflow. I used newrelic/nri-bundle, which I believe uses nri-kubernetes as a subchart, which I believe disables process metrics by default here.

Let me know if I'm misunderstanding something. It seems like a pretty complex piece of config and not exactly clear to an outsider what does what.

As this is entering Preview soon with related new capabilities, we need to ensure the config is valid and we don't break the Infra experiences.

Yup.

@jack-berg
Copy link
Contributor Author

As this is entering Preview soon with related new capabilities, we need to ensure the config is valid and we don't break the Infra experiences.

This repo really needs a smoke test as part of the build 😉

@jack-berg
Copy link
Contributor Author

Anything required to get this merged?

It appears that snyk validation is failing due to an authentication error unrelated to this PR.

Copy link
Contributor

@rubenruizdegauna rubenruizdegauna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jack-berg jack-berg force-pushed the align-host-monitoring-config branch from c37868a to 7247fb2 Compare July 22, 2024 20:54
@jack-berg
Copy link
Contributor Author

This repo has a merge requirement that all commits are signed, so I recommitted signed commits and force pushed.

@rubenruizdegauna do you have merge permissions?

@rubenruizdegauna rubenruizdegauna merged commit e09d065 into newrelic:main Jul 23, 2024
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants