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

Add redis metrics semantic conventions #2547

Closed

Conversation

codeboten
Copy link
Contributor

Changes

The following adds semantic conventions for redis metrics.

@codeboten codeboten force-pushed the codeboten/redis-metrics branch from 611ef9a to d684a66 Compare May 17, 2022 21:44
@codeboten codeboten marked this pull request as ready for review May 17, 2022 21:44
@codeboten codeboten requested review from a team May 17, 2022 21:44
@codeboten
Copy link
Contributor Author

This replaces #2525

@reyang
Copy link
Member

reyang commented May 18, 2022

Related to #2521 and #2525. FYI @haboy52587.

@reyang reyang added the area:semantic-conventions Related to semantic conventions label May 18, 2022
@arminru arminru added spec:metrics Related to the specification/metrics directory semconv:database labels May 18, 2022
@tigrannajaryan
Copy link
Member

@pmcollins @dmitryax as codeowners of Redis receiver in the Collector it would be great if you review this PR.

| Name | Instrument | Value type | Unit | Unit ([UCUM](../README.md#instrument-units)) | Description | Attribute Key | Attribute Values |
| ---------------------------------------------- | ------------- | ---------- | ------------ | -------------------------------------------- | --------------------------------------------------------------------- | ------------- | ---------------- |
| db.redis.uptime | Counter | Int64 | seconds | `{s}` | Number of seconds since Redis server start | | |
| db.redis.cpu.time | Counter | Double | seconds | `{s}` | System CPU consumed by the Redis server in seconds since server start | `state` | |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why having another metric for "cpu.time" rather than "process.cpu.time" with a "db.system" attribute on the resource?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bogdandrutu is there a guideline around when to add a new metric vs when to add a label to an existing metric? Just want to get some clarity on what other metrics we should be considering this with.

For example the connections received/rejected, uptime and all memory metrics feel like they may fall into this category as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would go this path anytime is possible :) Maybe we need some documentation about this?

| db.redis.memory.lua | Gauge | Int64 | bytes | `{by}` | Number of bytes used by the Lua engine | | |
| db.redis.memory.fragmentation_ratio | Gauge | Double | | | Ratio between used_memory_rss and used_memory | | |
| db.redis.rdb.changes_since_last_save | UpDownCounter | Int64 | changes | `{changes}` | Number of changes since the last dump | | |
| db.redis.commands | Gauge | Int64 | operations | `{ops}/s` | Number of commands processed per second | | |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to have this when we have the db.redis.commands.processed as counter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, the rate could be calculated from the db.redis.commands.processed counter

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then why not removing it?

| db.redis.connections.rejected | Counter | Int64 | connections | `{connections}` | Number of connections rejected because of maxclients limit | | |
| db.redis.memory.used | Gauge | Int64 | bytes | `{by}` | Total number of bytes allocated by Redis using its allocator | | |
| db.redis.memory.peak | Gauge | Int64 | bytes | `{by}` | Peak memory consumed by Redis | | |
| db.redis.memory.rss | Gauge | Int64 | bytes | `{by}` | Number of bytes that Redis allocated as seen by the operating system | | |
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this could be a more generic metric ie. process.memory.rss

| db.redis.memory.lua | Gauge | Int64 | bytes | `{by}` | Number of bytes used by the Lua engine | | |
| db.redis.memory.fragmentation_ratio | Gauge | Double | | | Ratio between used_memory_rss and used_memory | | |
| db.redis.rdb.changes_since_last_save | UpDownCounter | Int64 | changes | `{changes}` | Number of changes since the last dump | | |
| db.redis.commands | Gauge | Int64 | operations | `{ops}/s` | Number of commands processed per second | | |
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, the rate could be calculated from the db.redis.commands.processed counter

| db.redis.rdb.changes_since_last_save | UpDownCounter | Int64 | changes | `{changes}` | Number of changes since the last dump | | |
| db.redis.commands | Gauge | Int64 | operations | `{ops}/s` | Number of commands processed per second | | |
| db.redis.commands.processed | Counter | Int64 | operations | `{ops}` | Total number of commands processed by the server | | |
| db.redis.net.input | Counter | Int64 | bytes | `{by}` | The total number of bytes read from the network | | |
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

net input/output could be generalized as process metrics.

codeboten pushed a commit to codeboten/opentelemetry-collector-contrib that referenced this pull request May 24, 2022
As per the suggestions made in open-telemetry/opentelemetry-specification#2547, changing the following metrics:
- redis.cpu.time -> process.cpu.time
@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label May 27, 2022
Alex Boten added 3 commits May 31, 2022 08:09
The following adds semantic conventions for redis metrics.
@codeboten codeboten force-pushed the codeboten/redis-metrics branch from 5412274 to eb02c4c Compare May 31, 2022 15:28
@jmacd jmacd removed the Stale label Jun 1, 2022
@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Jun 10, 2022
@github-actions
Copy link

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Jun 17, 2022
@hughesjj hughesjj mentioned this pull request Sep 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:semantic-conventions Related to semantic conventions semconv:database spec:metrics Related to the specification/metrics directory Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants