open-telemetry / opentelemetry-collector-contrib

Contrib repository for the OpenTelemetry Collector
https://opentelemetry.io
Apache License 2.0
2.97k stars 2.3k forks source link

Switching to resource_attributes enabled all resource attributes instead of opt-in like before #28832

Open Tadimsky opened 11 months ago

Tadimsky commented 11 months ago

Component(s)

processor/resourcedetection

What happened?

Description

I got a warning in the collector to switch over to using resource_attributes instead of attributes for my ECS resource detection. I did so and then noticed that my metric cardinality increased significantly.

It looks like resource_attributes changed the behaviour from opt-in to opt-out and for ECS everything is enabled by default.

PR: https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/23253

ECS metadata file: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/processor/resourcedetectionprocessor/internal/aws/ecs/metadata.yaml

This was not called out in the Changelog that it would enable all of these properties by default. It also now means everyone using the resource_attributes has to keep up to date on the list of attributes and disable them if any more are added. This seems like it would be a breaking change each time we add a new property.

Should we disable them all by default? Or at least match the spec described here: https://github.com/open-telemetry/semantic-conventions/blob/eacb63da1139c8802448614c040d40cd321d6602/specification/resource/semantic_conventions/cloud_provider/aws/ecs.md

Collector version

v0.34.0

Environment information

Environment

ECS Fargate

OpenTelemetry Collector configuration

No response

Log output

No response

Additional context

No response

github-actions[bot] commented 11 months ago

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

dashpole commented 11 months ago

@dmitryax @mx-psi

dmitryax commented 11 months ago

I believe we need to reconsider the default list of attributes.

Another issue is the config generated by mdatagen only supports going from the default set of metrics/attributes. I think we need to support a way to disable or enable all of them with a one line config option. Then users can opt in or opt out. I need to think more about how the config interface would look like. Any suggestions are welcome.

mx-psi commented 10 months ago

Should we disable them all by default? Or at least match the spec described here: https://github.com/open-telemetry/semantic-conventions/blob/eacb63da1139c8802448614c040d40cd321d6602/specification/resource/semantic_conventions/cloud_provider/aws/ecs.md

Not sure what you mean here with 'match the spec'. AFAICT all attributes added by the detector are in the specification:

All of them are marked as 'Recommended', which means that:

Instrumentations SHOULD add the attribute by default if it's readily available and can be efficiently populated.

and that we should avoid adding it

due to performance, security, privacy, or other consideration[s]

I think the current behavior is the right choice with regards to what the specification says. Do you think there are reasons to not add a specific attribute (performance, security or otherwise)?

github-actions[bot] commented 8 months ago

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

github-actions[bot] commented 5 months ago

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

github-actions[bot] commented 3 months ago

This issue has been closed as inactive because it has been stale for 120 days with no activity.

github-actions[bot] commented 1 month ago

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.