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 resource #2836

Closed
wants to merge 2 commits into from
Closed

Conversation

hughesjj
Copy link

@hughesjj hughesjj commented Sep 26, 2022

Changes

Adds a Redis resource for db.instance type as the first step to collecting Redis metrics (not traces). See #2840

HUGE note of context: These changes are resurrected from a long-closed but much discussed PR. Regrettably, the original author is not here to Shepard these changes to release, but I believe I've got his work working again.

I don't see anyone having implemented such functionality since, but some have tried.
https://github.com/open-telemetry/opentelemetry-specification/pulls?q=is%3Apr+redis+

Testing

make all # specifically the table-check and table-generation

Related Links

update CHANGELOG.md and README.md for redis resource semantic convention

added db.redis.instance semantic convention

added db.redis.instance attribute
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 26, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@@ -532,6 +532,8 @@ release.
conventions. ([#2272](https://github.com/open-telemetry/opentelemetry-specification/pull/2272))
- Add opentracing.ref_type semantic convention.
([#2297](https://github.com/open-telemetry/opentelemetry-specification/pull/2297))
- Add `db.redis.instance` Resource attribute in Redis.
Copy link
Author

Choose a reason for hiding this comment

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

I'll add this PR here before moving out of draft
#2836

@hughesjj hughesjj mentioned this pull request Sep 26, 2022
@hughesjj hughesjj marked this pull request as ready for review September 27, 2022 16:45
@hughesjj hughesjj requested review from a team September 27, 2022 16:45
@@ -89,7 +89,7 @@ release.
([#2676](https://github.com/open-telemetry/opentelemetry-specification/pull/2676))
- Align log SDK and API component naming
([#2768](https://github.com/open-telemetry/opentelemetry-specification/pull/2768)).
- Add the signal-specific OTEL_EXPORTER_OTLP_LOGS_* environment variables
- Add the signal-specific OTEL*EXPORTER_OTLP_LOGS*\* environment variables
Copy link
Member

Choose a reason for hiding this comment

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

Please don't make changes that are not directly relevant to the PR. It would be great if you could submit these fixes as a separate PR.

@joaopgrassi
Copy link
Member

joaopgrassi commented Oct 4, 2022

This issue open-telemetry/semantic-conventions#713 might affect/be relevant here

@@ -0,0 +1,14 @@
groups:
- id: redis
Copy link
Member

@joaopgrassi joaopgrassi Oct 4, 2022

Choose a reason for hiding this comment

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

Isn't this PR having the same "issue" as the original PR: #2145 (comment)

Did you use this approach as a "workaround" for the conflict as here #2145 (comment)

or is the intention to have redis as a "top-level" entry? If it's because it's a workaround, then maybe we should look into the generator more, or find another way to generate it.

@@ -0,0 +1,13 @@
# Redis
Copy link
Member

Choose a reason for hiding this comment

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

We already have Redis-specific conventions in database.md. Do we need to introduce a new file just for Redis?

Copy link
Author

Choose a reason for hiding this comment

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

After talking with Joao and verifying on my end, it looks like you're correct and we can indeed use the existing file even though it's under a different "path" with the "traces".

@hughesjj
Copy link
Author

Closing due to

  1. It's un-needed, as we can use the existing definitions in the trace folder
  2. It's probably better to wait for the new SIG to decide how to "organize" the space rather than force something not strictly needed through that might need rework in a week or so anyway.

@hughesjj hughesjj closed this Oct 11, 2022
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

Successfully merging this pull request may close these issues.

4 participants