search

open-telemetry / opentelemetry-collector-contrib

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

Incorrect result for memcached.operation_hit_ratio #30695

Open vijayaggarwal opened 7 months ago

vijayaggarwal commented 7 months ago

Component(s)

receiver/memcached

What happened?

Description

The memcached.operation_hit_ratio actually calculates miss ratio instead of hit ratio.

The function calculateHitRatio() (https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/v0.92.0/receiver/memcachedreceiver/scraper.go#L175) expects arguments in the order (misses, hits) but is being passed arguments in the order (hits, misses). Thus, it actually calculates miss ratio instead of hit ratio.

Also, on a separate note, this metric calculates the ratio since the memcached instance start/restart, and hence the result changes on instance restart. This is counterintuitive in my opinion, as I would expect the metric to represent "recent" hit ratio. Since there is not universal definition of recent, I feel this metric should be deprecated and people should be encouraged to calculate the hit ratio from the memcached.operations metric as per their requirement.

Collector version

all recent versions

Environment information

No response

OpenTelemetry Collector configuration

No response

Log output

No response

Additional context

No response

github-actions[bot] commented 7 months ago

Pinging code owners:

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

djaglowski commented 7 months ago

Thanks for reporting @vijayaggarwal. I'm in support of deprecating the metric.

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.

kernelpanic77 commented 3 months ago

@djaglowski Can I take this up ?

djaglowski commented 3 months ago

Yes, thanks @kernelpanic77

kernelpanic77 commented 3 months ago

@djaglowski Should we deprecate this metric or disable it in the metadata.yaml of memcachedreciever ?

djaglowski commented 3 months ago

@kernelpanic77 we should follow the process described here.

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.