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 specification #2525

Closed
wants to merge 3 commits into from

Conversation

haboy52587
Copy link

Fixes #

Changes

Please provide a brief description of the changes here. Update the
CHANGELOG.md for non-trivial changes. If CHANGELOG.md is updated,
also be sure to update spec-compliance-matrix.md if necessary.

Related issues #

Related oteps #

@haboy52587 haboy52587 requested review from a team May 5, 2022 21:54
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented May 5, 2022

CLA Missing ID CLA Not Signed

@haboy52587 haboy52587 changed the title opentelemetry-specification add redis spec Add redis specification May 5, 2022

| Name | Instrument | Units | Description | Attribute Keys | Attributes |
|-------------------------------|----------------------------|--------------|-------------|-|-|
|`redis.uptime`|Asynchronous Counter|seconds|Number of seconds since Redis server start|
Copy link
Member

@reyang reyang May 6, 2022

Choose a reason for hiding this comment

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

@haboy52587 thanks for your contribution!

Please refer to this PR #2458 where we've decided to focus on "what" do we get from the Instrument, rather than "how". In a nutshell, consider removing "Asynchronous" / "Synchronous".

|`redis.keyspace.hits`|Asynchronous Counter||Number of successful lookup of keys in the main dictionary| | |
|`redis.keyspace.misses`|Asynchronous Counter||Number of failed lookup of keys in the main dictionary| | |
|`redis.latest_fork`|Asynchronous Gauge|microseconds|Duration of the latest fork operation| | |
|`redis.slaves.connected`|Asynchronous UpDownCounter||Number of connected replicas| | |
Copy link
Member

Choose a reason for hiding this comment

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

Could this be called redis.replicas.connected? (given most oss projects are moving from master branch to main branch, I guess in the long run Redis would be inclined to avoid "slaves")

@@ -0,0 +1,46 @@
# Semantic conventions for database metrics
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Semantic conventions for database metrics
# Semantic conventions for Redis metrics

reyang
reyang previously approved these changes May 6, 2022
Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

The overall direction looks good to me once https://github.com/open-telemetry/opentelemetry-specification/pull/2525/files#r866476318 is addressed.

I'm not Redis expert, it'll be great to get someone who is familiar with Redis (or even better, could we invite folks from the Redis community?). Not necessarily blocking given the status of the doc is Experimental.

@reyang reyang added area:semantic-conventions Related to semantic conventions spec:metrics Related to the specification/metrics directory labels May 6, 2022
@reyang
Copy link
Member

reyang commented May 6, 2022

@haboy52587 please update the CHANGELOG.

@reyang
Copy link
Member

reyang commented May 6, 2022

@haboy52587 please clear the CLA.

@reyang reyang dismissed their stale review May 6, 2022 04:19

Actually, please clear the CLA first.

Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

@haboy52587 I commented your previous PR.

Still missing:

@carlosalberto
Copy link
Contributor

@tigrannajaryan Actually the existing ones are for traces, and this is for metrics.

Other than that, there are some metric that looks very Redis specific, e.g. redis.clients.max_output_buffer or redis.memory.lua. Adding this kind would make sense still?

@tigrannajaryan
Copy link
Member

@tigrannajaryan Actually the existing ones are for traces, and this is for metrics.

Other than that, there are some metric that looks very Redis specific, e.g. redis.clients.max_output_buffer or redis.memory.lua. Adding this kind would make sense still?

We use db.redis. prefix for Redis traces. I think we should do the same for metrics.

@carlosalberto
Copy link
Contributor

I think we should do the same for metrics.

Oh, yes, let's definitely go with the same prefix.

@reyang
Copy link
Member

reyang commented May 9, 2022

I think we should do the same for metrics.

Oh, yes, let's definitely go with the same prefix.

Or maybe we should remove db. from Redis semantic convention (why is Redis considered db?). #2485 (comment)

@tigrannajaryan
Copy link
Member

Or maybe we should remove db. from Redis semantic convention (why is Redis considered db?). #2485 (comment)

From https://github.com/redis/redis:

About
Redis is an in-memory database that persists on disk. The data model is key-value, but many different kind of values >are supported: Strings, Lists, Sets, Sorted Sets, Hashes, Streams, HyperLogLogs, Bitmaps.

Also from https://redis.com/:

Redis Enterprise is simply the best version of Redis, the most loved database in the world.

@carlosalberto
Copy link
Contributor

Closing this one as the author won't be continuing with this - also @codeboten will take on this one.

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 spec:metrics Related to the specification/metrics directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants