-
Notifications
You must be signed in to change notification settings - Fork 26
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
Handle system.process.* for OTel translated metrics #309
Conversation
// `system.process.cmdline` is same as the ECS field `process.command_line` | ||
// however, Kibana curated UIs requires this field to work. In addition, | ||
// the current Kibana code will not work if this field is added to documents | ||
// with `system.process.memory.rss.pct` and other metrics required in the | ||
// Processes tab of the Kibana hostmetrics UI. Due to this, we have to process | ||
// the datapoint field added by the opentelemetry-lib instead of directly | ||
// processing the OTel semconv resource attribute `process.command_line`. |
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.
@felixbarny @axw This means that we would not be able to remove the attributes added at https://github.com/elastic/opentelemetry-lib/blob/54a9569b27e3f11edc0d660dc79ac865256f3cd3/remappers/hostmetrics/process.go#L271-L295 and rely on the translations performed in APM/esexporter without UI fixing such hard dependencies. One such case I have explained here (elastic only link).
input/otlp/metrics_test.go
Outdated
{ | ||
Name: "system.load.5", |
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.
Question: Why is this here in the first place with no value, and why is it not present now after the PR?
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.
It is due to how otel lib behaves. There was a fix in v0.6.0 but that had to reverted in v0.6.1 so these lines need to be added back. I have added a comment explaining why it is there pasting below for reference:
// TODO (lahsivjar): Opentelemetry lib currently adds all metrics, even
// though the source metrics is not present. Due to this all the metrics
// need to be asserted making the tests brittle. This should be fixed by
// https://github.com/elastic/opentelemetry-lib/issues/6
system.process.*
fields are required on the process metrics to make the UI work. These are non-ecs fields but must haves for the Processes tab in the Host metrics UI to work.All these fields are processed as data point attributes added to the remapped OTel metrics due to UI constraints (explained in code comments).
Related to:
To be merged after elastic/opentelemetry-lib#35