open-telemetry / opentelemetry-collector

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

Consistent scope name scheme #9494

Closed codeboten closed 1 month ago

codeboten commented 7 months ago

The meter name (that is translated into scope name) is currently inconsistent in this repository. Some components instantiate a meter with a short form like the following, which is consistent w/ the contrib repo:

otelcol/otlphttp
otelcol/ballast

While others use a fully qualified name, which is consistent w/ go instrumentations:

go.opentelemetry.io/collector/service/process_telemetry
go.opentelemetry.io/collector/exporterhelper
go.opentelemetry.io/collector/obsreport/processor
go.opentelemetry.io/collector/processor/batchprocessor

These should be consistent.

bogdandrutu commented 7 months ago

go.opentelemetry.io/collector/service/process_telemetry

here is probably just otelcol?

go.opentelemetry.io/collector/exporterhelper

here we should calculate the scope name at runtime based on the exporter that uses the helper correct?

go.opentelemetry.io/collector/obsreport/processor

same as above.

codeboten commented 7 months ago

@bogdandrutu are you in favour of keeping the short form or the long form scope name? or both?

here is probably just otelcol?

I guess it depends on how specific we want this scope to be. Would it make sense to scope it to the individual go modules that are published? This way it would allow a consumer of the telemetry to identify package versions and maybe if there's a problem with a specific module version, they would be able look at the scope information to narrow down their search?

andrzej-stencel commented 7 months ago

I like the fully qualified name a bit more, as it is more descriptive. We also have some components of different types that have the same name, which would result in same short scope name, for example:

I lean towards the opinion that e.g. exporterhelper should use the scope of the exporter that it is embedded in. I'm not convinced that we should surface Go module names in the metrics exposed to users - this feels like an implementation detail. We don't want the metrics to change when we move functionality from one module to another, which might otherwise be a purely non-functional refactoring.

dashpole commented 7 months ago

I've liked when the scope name is actually a link to the godocs for the package (e.g. go.opentelemetry.io/collector/exporter/exporterhelper). It removes any ambiguity for someone trying to figure out where the metric is defined, or what it is about.

bogdandrutu commented 7 months ago

I've liked when the scope name is actually a link to the godocs for the package (e.g. go.opentelemetry.io/collector/exporter/exporterhelper). It removes any ambiguity for someone trying to figure out where the metric is defined, or what it is about.

The problem with this, is that we need also a attribute to describe the component (exporter name) for which you record the metric when is a shared library like exporterhelper. I do understand that using go.opentelemetry.io/collector/exporter/exporterhelper may help but comes with that downside, which we may be fine.

dmitryax commented 7 months ago

Looks like this is resolved for the core repo. We still use short version in contrib, e.g. otelcol/countconnector. We need to update that as well. I see 3 options:

  1. Keep the same prefix: go.opentelemetry.io/collector/connector/countconnector
  2. Use current repo address: github.com/open-telemetry/opentelemetry-collector-contrib/connector/countconnector
  3. Use go.opentelemetry.io/collector-contrib/connector/countconnector assuming we are going to migrate the modules to vanity URLs in contrib https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/21469

What do you think?

andrzej-stencel commented 6 months ago

Option 3. makes most sense to me.

@dmitryax perhaps add emojis to each option (e.g. 1 :tada: , 2 :heart: , 3 :rocket:) to enable voting on your comment. :slightly_smiling_face:

codeboten commented 1 month ago

This has been done for core and contrib components.