open-telemetry / opentelemetry-collector

OpenTelemetry Collector
https://opentelemetry.io
Apache License 2.0
4.28k stars 1.41k forks source link

Define attributes for internal metrics in metadata.yaml #10801

Open dmitryax opened 1 month ago

dmitryax commented 1 month ago

Define all the attributes for internal metrics in the metadata.yaml similar to what we do for the scraper metrics.

Those attribute definitions should be used to generate documentation and helpers that replace the directly exposed counters with Record... functions.

For example:

Instead of

type TelemetryBuilder struct {
    ExporterSentSpans metric.Int64Counter
}

we would generate

type TelemetryBuilder struct {
    exporterSentSpans   metric.Int64Counter
}

// RecordExporterSentSpans adds a value to the ExporterSentSpans metric.
func (builder *TelemetryBuilder) RecordExporterSentSpans(ctx context.Context, value int64, exporter string) {
    builder.exporterSentSpans.Add(context.Background(), value, metric.WithAttributeSet(attribute.NewSet(attribute.String("exporter", exporter))))
}
dmitryax commented 1 month ago

@codeboten, please let me know if it makes sense

codeboten commented 1 month ago

100% i would like to move in that direction for all internal telemetry as well

crobert-1 commented 1 month ago

Just to confirm with the assignment, I'll work on this 👍

crobert-1 commented 2 weeks ago

Started working on this, just want to bring up a limitation of the upstream go.opentelemetry.io/otel/attribute package. As described in this issue's description, attributes are added to a telemetry metric using the go.opentelemetry.io/otel/attribute package's functionality of attribute.NewSet, passing in the correct KeyValue information.

However, KeyValue only supports a subset of valid attribute types that our metadaya.yaml supports.

From mdatagen's metadata schema: https://github.com/open-telemetry/opentelemetry-collector/blob/505819e63ae2b615d7c57c9cfe2b4325ae82dc4c/cmd/mdatagen/metadata-schema.yaml#L72

Types supported by KeyValue can be seen here.

The missing types are as follows: bytes, slice, and map. I believe this is an existing limitation, unrelated to this change. However, it may be good to document this limitation in the metadata schema file.

Edit: I've submitted #10997 to clearly state this limitation in the schema.

crobert-1 commented 1 week ago

I've got most of the functionality working, but I'd like to make sure it's tested more thoroughly before going forward. My current plan is as follows:

  1. Introduce telemetry metric test to test each telemetry metric. This test is written manually, and the PR's purpose will be to agree on format of test.
  2. Convert test to be generated by mdatagen. This will result in telemetry metric tests for each component that has telemetry metrics defined in their metadata.yaml.
  3. Introduce behavior described in this issue.

My goal is to be able to have tests that ensure existing behavior of recording telemetry metrics and their attributes doesn't change as a result of this.