Added specification for OpenTelemetry metric shipping

[JonasKunz]

• May the instrumentation collect sensitive information, such as secrets or PII (ex. in headers)?
  • Yes
    • Add a section to the spec how agents should apply sanitization (such as sanitize_field_names)
  • No
    • Why? -> The agent only collects what the user specifies, therefore I would not see sanitization as a requirement from our side. In addition PII in labels is a terrible idea anyway due to cardinality.
  • n/a
• Create PR as draft
• Approval by at least one other agent
• Mark as Ready for Review (automatically requests reviews from all agents and PM via CODEOWNERS)
  • Remove PM from reviewers if impact on product is negligible
  • Remove agents from reviewers if the change is not relevant for them
• Approved by at least 2 agents + PM (if relevant)
• Merge after 7 days passed without objections
  To auto-merge the PR, add /schedule YYYY-MM-DD to the PR description.
• Create implementation issues through the meta issue template (this will automate issue creation for individual agents)
• If this spec adds a new dynamic config option, add it to central config.

[apmmachine]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Start Time: 2023-03-31T05:49:00.086+0000

• Duration: 5 min 41 sec

[JonasKunz]

The current spec text here is supposed to act more as a starting point for a discussion.

The java agent already has the dedot_custom_metrics configuration, which is on by default. This currently only affects Micrometer metrics, as this is the only way of collecting custom metrics atm.

On the otherside, the APM server does not perform a dedoting of metric names when receiving metrics via OTLP.

This means with the current proposal, the transition from sending metrics via the OTLP exporter vs shipping them via the agent would not be seamless: because dedot_custom_metrics is on by default, the metric names would change.

There are some alternatives to consider:

• Add a dedot_custom_metrics option to the APM-server and make it on by default (breaking change)
• Change the default dedot_custom_metrics in the java agent to be disabled by default (breaking change)
• Separate the dedot_custom_metrics in the java agent into two different config options, one for Otel and one for micrometer. Terrible idea imo, because I don't see a use case where these two would be configured differently.

Please use this discussion to vote for what you think is best or to propose alternative solutions.

[felixbarny]

Related: https://github.com/elastic/apm-dev/issues/856 (internal)

[JonasKunz]

After some more thinking and offline discussions, what about the following "optimistic" solution:
In long/mid term, we want to get rid of the dedoting in general by mapping metricsets in elasticsearch using subobjects: false. Until we get there, we should aim to minimize breaking-changes.

So for all agents which do not have the dedot_custom_metrics config yet (=everyone but java):

• Agents MAY add the dedot_custom_metrics configuration option, but don't need to (maybe added later ondemand if issues with users arise)
• If dedot_custom_metrics is added, the default MUST be false -> no breaking change required later on when we support dots in names fully

For all agents which already have the dedot_custom_metrics configuration (= the java agent):

• Keep the configuration dedot_custom_metrics with its default as is and respect it for Otel metrics
• As soon as we support dots in metric names fully, switch the default to false (breaking but mitigable change)

Opinions?

[felixbarny]

SGTM but proposing a slight variation that minimizes breaking changes in the future: Ignore the dedot_custom_metrics flag for OTel metrics in the Java agent and not implement it in other agents. As this can lead to data loss, we probably can't GA the feature (debatable as we did GA OTLP metrics intake). Work on support for subobjects: false: We could consider flattening the metric documents in APM Server, try getting elastic/elasticsearch#88934 prioritized or contribute this feature to ES (👨‍🚀⏳?).

Then we only need to introduce the breaking change of changing the default for dedot_custom_metrics for Micrometer. I suppose that we don't really need the dedot_custom_metrics flag at that time anymore so we can deprecate it when we change the default to false and eventually remove it.

[JonasKunz]

we probably can't GA the feature (debatable as we did GA OTLP metrics intake)

Due to the fact that we create separate metricsets for the Otel data instead of merging them with e.g. the agent collected jvm metricsets I think the mapping conflicts would not be too bad, at least not worse than for the OTLP intake. E.g. we would only loose Otel metrics data, all other metrics would continue to work normally. So I'm not sure whether this really is a GA blocker.

[JonasKunz]

This is another difference to how the APM server currently does things when receiving metric data via OTLP.
The APM-Server will create a different metricset per instrumentation scope, but won't add any label to distinguish them (to my knowledge).
This means that when displaying the metrics in Kibana, the metrics with the same name will get mixed up even if they come from different instrumentation scopes.

Would be great if @elastic/apm-server could clarify here if I'm missing something or if my proposed label makes sense and we maybe should add it on the OTLP intake side aswell.

[trentm]

Some notes on this.

1. https://opentelemetry.io/docs/reference/specification/metrics/api/#get-a-meter and https://opentelemetry.io/docs/reference/specification/glossary/#instrumentation-scope are not very clear, but one could read from those that the instrumentation scope version (and maybe schema_url) fields may also be relevant for uniquely identifying a scope.
2. FWIW, looking at the Prometheus exporter from the OTel JS SDK, the instrumentation scope is not included anywhere in the output. I created a JS script that has two my_counter metrics with diff instrumentation scopes:

...
const meter = otel.metrics.getMeter('my-meter') // 'my-meter' is the instrumentation scope name
const counter = meter.createCounter('my_counter', { description: 'My Counter' })

const meter2 = otel.metrics.getMeter('my-meter2')
const counter2 = meter2.createCounter('my_counter', { description: 'My Counter 2' })

setInterval(() => {
  counter.add(1)
  counter2.add(1)
}, 1000)

The resulting exported Prom metrics:

% curl localhost:3002/metrics
# HELP target_info Target metadata
# TYPE target_info gauge
target_info{service_name="use-otel-prom",telemetry_sdk_language="nodejs",telemetry_sdk_name="opentelemetry",telemetry_sdk_version="1.9.1"} 1
# HELP my_counter_total My Counter
# TYPE my_counter_total counter
my_counter_total 317 1679701732718
# HELP my_counter_total My Counter 2
# TYPE my_counter_total counter
my_counter_total 317 1679701732718

So, I wonder if this is the conflict we could later come back if/when there is a real use case.

3. If we were to add an intake field or label for this, https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/common/mapping-to-non-otlp.md#instrumentationscope describes otel.scope.name and otel.scope.version. I'm not sure if that applies here.

[JonasKunz]

https://opentelemetry.io/docs/reference/specification/metrics/api/#get-a-meter and https://opentelemetry.io/docs/reference/specification/glossary/#instrumentation-scope are not very clear, but one could read from those that the instrumentation scope version (and maybe schema_url) fields may also be relevant for uniquely identifying a scope.

It is true that the instrumentation scope name alone might not be good enough to prevent clashes in all cases. It is good enough to prevent cross-library clashes (e.g. library A and B happen to export a metric with the same name) but won't handle the case where you have multiple versions of the same library with breaking changes in their metrics.

My thought here was to start simple and to leave the option open to maybe just use a single label (name and version concatenated) instead of adding both immediately.

FWIW, looking at the Prometheus exporter from the OTel JS SDK, the instrumentation scope is not included anywhere in the output. I created a JS script that has two my_counter metrics with diff instrumentation scopes
So, I wonder if this is the conflict we could later come back if/when there is a real use case.

Interesting observation that this is currently not even handled by the Otel prometheus exporter.
We can definitely just ignore the InstrumentationScope for now and add a label later on, as it would be a non-breaking change.

We should however imo keep in the spec that InstrumentationScopes have to use a different metricsets, even if they have the same labels. Otherwise we would need to decide on a "winner" metric if we encounter a clash.

[trentm]

In the meeting today we discussed using otel.scope.name and otel.scope.version and perhaps adding those at top-level fields (@gregkalapos volunteered).

@Mpdreamz mentioned (if I understood correctly) that metricbeat uses ECS observer.name (https://www.elastic.co/guide/en/ecs/current/ecs-observer.html#field-observer-name) for similar purpose?

[JonasKunz]

I've left the concrete addition of the fields for otel.scope.name and otel.scope.version for the future, so that this doesn't block merging this spec:

Agents MUST report metrics from different instrumentation scopes in separate metricsets to avoid naming conflicts at collection time. This separation MUST be done based on the instrumentation scope name and version. In the future, we might add dedicated intake fields to metricsets for differentiation them based on the instrumentation scope identifiers.

[felixbarny]

For the OTel tracing bridge, we introduced an otel.attributes property in the intake v2 spec: https://github.com/elastic/apm/blob/main/specs/agents/tracing-api-otel.md#attributes-mapping.

APM Server then maps known attributes to ECS and unknown attributes to labels. I wonder if we should do the same for metrics. Does APM Server currently map OTel metric attributes to ECS fields?

Maybe this isn't as relevant to metrics as it is to traces if the metric attributes are mostly used for custom user-defined attributes.

[simitt]

The server maps the metadata and some known and defined metrics.

[gregkalapos]

I did a first parse, great progress on this 👍 .

Few, minor comments below.

[SylvainJuge]

[minor] Maybe worth mentioning that this is different from the OTel bridge where the agent overrides the values returned by GlobalOpenTelemetry with its own implementation. Here with metrics SDK the agent just adds an extra exporter at the last step of the metrics pipeline (potentially replacing another sending metrics to another backend).

[JonasKunz]

Maybe worth mentioning that this is different from the OTel bridge where the agent overrides the values returned by GlobalOpenTelemetry with its own implementation.

Isn't GlobalOpenTelemetry something java-specific? It is just the java approach of binding the opentelemetry API to an implementation (e.g. an SDK or our bridge).

Here with metrics SDK the agent just adds an extra exporter at the last step of the metrics pipeline (potentially replacing another sending metrics to another backend)

I don't quiet understand what you mean here? When we add our exporter, we don't replace existing exporters on the user-provided metrics SDK instance. Instead, both exporters live happily side by side.