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

Consistent scope name scheme #9494

Open
codeboten opened this issue Feb 6, 2024 · 7 comments
Open

Consistent scope name scheme #9494

codeboten opened this issue Feb 6, 2024 · 7 comments

Comments

@codeboten
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor Author

@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
Copy link
Member

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
Copy link
Contributor

dashpole commented Feb 7, 2024

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
Copy link
Member

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.

codeboten pushed a commit to codeboten/opentelemetry-collector that referenced this issue Feb 12, 2024
This is part of open-telemetry#9494

Signed-off-by: Alex Boten <aboten@lightstep.com>
codeboten pushed a commit that referenced this issue Feb 14, 2024
This is part of #9494

---------

Signed-off-by: Alex Boten <aboten@lightstep.com>
@dmitryax
Copy link
Member

dmitryax commented Mar 5, 2024

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 Migrate the modules to use vanity url go.opentelemetry.io/collector-contrib opentelemetry-collector-contrib#21469

What do you think?

dmitryax added a commit that referenced this issue Mar 5, 2024
Don't use hardcoded "go.opentelemetry.io/collector" prefix. Provide a
way to specify the `scope_name` in metadata.yaml. If not provided, try
to use the go package name.

Updates
#9494
dmitryax added a commit to dmitryax/opentelemetry-collector-contrib that referenced this issue Mar 5, 2024
This change migrates `generate` make target from using deprecated cmd/mdatagen defined in this repo to mdategen defined in core repository.

In order to avoid breaking changes for end users, we keep the scope names used in this repo the same as before. This required defining them explicitly in metadata.yaml files. We can update them after open-telemetry/opentelemetry-collector#9494 and open-telemetry#21469 are resolver.

Taking the opportunity that that the scope names can be explicitly defined, this PR also updates missing scope names for extensions with inconsistent package names e.g.: awsproxy and jaegerremotesampling. It's not a breaking change because the generated meter and tracer are not being used yet.
dmitryax added a commit to dmitryax/opentelemetry-collector-contrib that referenced this issue Mar 5, 2024
This change migrates `generate` make target from using deprecated cmd/mdatagen defined in this repo to mdategen defined in core repository.

In order to avoid breaking changes for end users, we keep the scope names used in this repo the same as before. This required defining them explicitly in metadata.yaml files. We can update them after open-telemetry/opentelemetry-collector#9494 and open-telemetry#21469 are resolver.

Taking the opportunity that that the scope names can be explicitly defined, this PR also updates missing scope names for extensions with inconsistent package names e.g.: awsproxy and jaegerremotesampling. It's not a breaking change because the generated meter and tracer are not being used yet.
dmitryax added a commit to dmitryax/opentelemetry-collector-contrib that referenced this issue Mar 5, 2024
This change migrates `generate` make target from using deprecated cmd/mdatagen defined in this repo to mdategen defined in core repository.

In order to avoid breaking changes for end users, we keep the scope names used in this repo the same as before. This required defining them explicitly in metadata.yaml files. We can update them after open-telemetry/opentelemetry-collector#9494 and open-telemetry#21469 are resolver.

Taking the opportunity that that the scope names can be explicitly defined, this PR also updates missing scope names for extensions with inconsistent package names e.g.: awsproxy and jaegerremotesampling. It's not a breaking change because the generated meter and tracer are not being used yet.
dmitryax added a commit to dmitryax/opentelemetry-collector-contrib that referenced this issue Mar 6, 2024
This change migrates `generate` make target from using deprecated cmd/mdatagen defined in this repo to mdategen defined in core repository.

In order to avoid breaking changes for end users, we keep the scope names used in this repo the same as before. This required defining them explicitly in metadata.yaml files. We can update them after open-telemetry/opentelemetry-collector#9494 and open-telemetry#21469 are resolver.

Taking the opportunity that that the scope names can be explicitly defined, this PR also updates missing scope names for extensions with inconsistent package names e.g.: awsproxy and jaegerremotesampling. It's not a breaking change because the generated meter and tracer are not being used yet.
dmitryax added a commit to dmitryax/opentelemetry-collector-contrib that referenced this issue Mar 6, 2024
This change migrates `generate` make target from using deprecated cmd/mdatagen defined in this repo to mdategen defined in core repository.

In order to avoid breaking changes for end users, we keep the scope names used in this repo the same as before. This required defining them explicitly in metadata.yaml files. We can update them after open-telemetry/opentelemetry-collector#9494 and open-telemetry#21469 are resolver.

Taking the opportunity that that the scope names can be explicitly defined, this PR also updates missing scope names for extensions with inconsistent package names e.g.: awsproxy and jaegerremotesampling. It's not a breaking change because the generated meter and tracer are not being used yet.
dmitryax added a commit to dmitryax/opentelemetry-collector-contrib that referenced this issue Mar 6, 2024
This change migrates `generate` make target from using deprecated cmd/mdatagen defined in this repo to mdategen defined in core repository.

In order to avoid breaking changes for end users, we keep the scope names used in this repo the same as before. This required defining them explicitly in metadata.yaml files. We can update them after open-telemetry/opentelemetry-collector#9494 and open-telemetry#21469 are resolver.

Taking the opportunity that that the scope names can be explicitly defined, this PR also updates missing scope names for extensions with inconsistent package names e.g.: awsproxy and jaegerremotesampling. It's not a breaking change because the generated meter and tracer are not being used yet.
dmitryax added a commit to open-telemetry/opentelemetry-collector-contrib that referenced this issue Mar 6, 2024
…#31609)

This change migrates `generate` make target from using the deprecated
`cmd/mdatagen` in this repository to mdategen defined in core
repository.

To avoid breaking changes for the end users, we keep the scope names
used in this repo as before. This required defining them explicitly in
metadata.yaml files. We can update them after
open-telemetry/opentelemetry-collector#9494
and
#21469
are resolved.

Taking the opportunity that the scope names can be explicitly defined,
this PR also updates missing scope names for extensions with
inconsistent package names e.g.: `awsproxy` and `jaegerremotesampling`.
It's not a breaking change because the generated meter and tracer are
not being used yet.

This change unblocks
#30495
DougManton pushed a commit to DougManton/opentelemetry-collector-contrib that referenced this issue Mar 13, 2024
…open-telemetry#31609)

This change migrates `generate` make target from using the deprecated
`cmd/mdatagen` in this repository to mdategen defined in core
repository.

To avoid breaking changes for the end users, we keep the scope names
used in this repo as before. This required defining them explicitly in
metadata.yaml files. We can update them after
open-telemetry/opentelemetry-collector#9494
and
open-telemetry#21469
are resolved.

Taking the opportunity that the scope names can be explicitly defined,
this PR also updates missing scope names for extensions with
inconsistent package names e.g.: `awsproxy` and `jaegerremotesampling`.
It's not a breaking change because the generated meter and tracer are
not being used yet.

This change unblocks
open-telemetry#30495
XinRanZhAWS pushed a commit to XinRanZhAWS/opentelemetry-collector-contrib that referenced this issue Mar 13, 2024
…open-telemetry#31609)

This change migrates `generate` make target from using the deprecated
`cmd/mdatagen` in this repository to mdategen defined in core
repository.

To avoid breaking changes for the end users, we keep the scope names
used in this repo as before. This required defining them explicitly in
metadata.yaml files. We can update them after
open-telemetry/opentelemetry-collector#9494
and
open-telemetry#21469
are resolved.

Taking the opportunity that the scope names can be explicitly defined,
this PR also updates missing scope names for extensions with
inconsistent package names e.g.: `awsproxy` and `jaegerremotesampling`.
It's not a breaking change because the generated meter and tracer are
not being used yet.

This change unblocks
open-telemetry#30495
@andrzej-stencel
Copy link
Member

Option 3. makes most sense to me.

@dmitryax perhaps add emojis to each option (e.g. 1 🎉 , 2 ❤️ , 3 🚀) to enable voting on your comment. 🙂

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

No branches or pull requests

5 participants