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

[receiver/redis] include server.address and server.port resource attributes #26707

Merged

Conversation

sigilioso
Copy link
Contributor

@sigilioso sigilioso commented Sep 15, 2023

Description:

Two new resource attributes have been added: server.address and server.port. This is change aligned with semantic conventions and allows users identifying a particular redis server.

Link to tracking Issue: #22044

Testing:

❯ go test -race -timeout 300s -parallel 4 --tags="",integration --count 1 ./...
ok      github.com/open-telemetry/opentelemetry-collector-contrib/receiver/redisreceiver        6.179s
ok      github.com/open-telemetry/opentelemetry-collector-contrib/receiver/redisreceiver/internal/metadata      1.464s
  • Added: new test to check error when the configuration endpoint is missing or invalid
  • Updated: integration test to check new resource attributes

Documentation:
New attribute documentation was generated using mdatagen

@sigilioso sigilioso requested a review from dmitryax as a code owner September 15, 2023 11:22
@sigilioso sigilioso requested a review from a team September 15, 2023 11:22
@github-actions github-actions bot added the receiver/redis Redis related issues label Sep 15, 2023
@github-actions github-actions bot requested a review from hughesjj September 15, 2023 11:22
@sigilioso sigilioso force-pushed the redis-receiver-server-address-and-port branch from 49cf5db to 234e0f2 Compare September 15, 2023 12:40
@github-actions github-actions bot added the cmd/otelcontribcol otelcontribcol command label Sep 15, 2023
@sigilioso
Copy link
Contributor Author

Hi @dmitryax, @hughesjj, I was wondering if you could provide some feedback. Do you consider that identifying redis server by including those attributes is useful?

@ceevaaa
Copy link

ceevaaa commented Oct 9, 2023

Hey guys,

I am facing a similar issue while collecting metrics from five different Redis instances.
There is no way to identify which metrics came from which instance. I am sure adding server and port in resource attributes will significantly help in uniquely identifying the Redis instance.

My question is, when is this PR going to get merged, and when is it expected to be released ?

Big thanks for all the hard-work <3.

server.port:
description: Redis server's port
enabled: true
type: string
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the port a string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could parse it to get the corresponding number, but it is a string when it is taken from the address

Copy link
Contributor

Choose a reason for hiding this comment

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

it's fine, let's leave it as is

@sigilioso sigilioso requested a review from atoulme October 16, 2023 08:13
@atoulme atoulme added the ready to merge Code review completed; ready to merge by maintainers label Oct 27, 2023
@TylerHelmuth
Copy link
Member

pinging @dmitryax and @hughesjj as code owners

@mx-psi mx-psi merged commit dbdb683 into open-telemetry:main Nov 14, 2023
83 checks passed
@github-actions github-actions bot added this to the next release milestone Nov 14, 2023
RoryCrispin pushed a commit to ClickHouse/opentelemetry-collector-contrib that referenced this pull request Nov 24, 2023
…ibutes (open-telemetry#26707)

**Description:** <Describe what has changed.>

Two new resource attributes have been added: `server.address` and
`server.port`. This is change aligned with [semantic
conventions](https://github.com/open-telemetry/opentelemetry-specification/blob/v1.25.0/specification/semantic-conventions.md?plain=1#L30)
and allows users identifying a particular redis server.


**Link to tracking Issue:** open-telemetry#22044

**Testing:** <Describe what testing was performed and which tests were
added.>

```
❯ go test -race -timeout 300s -parallel 4 --tags="",integration --count 1 ./...
ok      github.com/open-telemetry/opentelemetry-collector-contrib/receiver/redisreceiver        6.179s
ok      github.com/open-telemetry/opentelemetry-collector-contrib/receiver/redisreceiver/internal/metadata      1.464s
```

* Added: new test to check error when the configuration endpoint is
missing or invalid
* Updated: integration test to check new resource attributes

**Documentation:** <Describe the documentation added.>
New attribute documentation was generated using `mdatagen`

---------

Co-authored-by: Antoine Toulme <antoine@toulme.name>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmd/otelcontribcol otelcontribcol command ready to merge Code review completed; ready to merge by maintainers receiver/redis Redis related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants