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

Redis -- service.instance.id #22044

Closed
hughesjj opened this issue May 17, 2023 · 6 comments
Closed

Redis -- service.instance.id #22044

hughesjj opened this issue May 17, 2023 · 6 comments
Assignees
Labels
enhancement New feature or request receiver/redis Redis related issues Stale

Comments

@hughesjj
Copy link
Contributor

Component(s)

No response

Is your feature request related to a problem? Please describe.

This captures the spirit of some long closed issues now that we have better guidance on semantic conventions for identifying an "instance" (db or otherwise)

Describe the solution you'd like

I'd like to use service.instance.id to be populated with endpoint from the collector configuration.

Describe alternatives you've considered

We could also add more information like the db_index to the instance id but I don't really see that as a different "instance", and that would live better as either another attribute at the resource or metrics level, but open to discussion.

Additional context

No response

@hughesjj hughesjj added enhancement New feature or request needs triage New item requiring triage labels May 17, 2023
@atoulme atoulme added receiver/redis Redis related issues and removed needs triage New item requiring triage labels May 17, 2023
@github-actions
Copy link
Contributor

Pinging code owners for receiver/redis: @dmitryax. See Adding Labels via Comments if you do not have permissions to add labels yourself.

@github-actions
Copy link
Contributor

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.

@github-actions github-actions bot added the Stale label Jul 17, 2023
@gsanchezgavier
Copy link
Contributor

@hughesjj Have you consider [run_id]
(https://redis.io/commands/info/#:~:text=run_id%3A%20Random%20value%20identifying%20the%20Redis%20server%20(to%20be%20used%20by%20Sentinel%20and%20Cluster))?
It looks like it identifies the instance but I see that it is being regenerated each restart so I'm a bit concerned that it could lead to confusion.

Regarding the proposal how you would tackle the issue of having (duplicated) localhost as the endpoint in the config in case the Redis instance being monitored is in the same Host as the Collector?

@github-actions github-actions bot removed the Stale label Aug 15, 2023
@hughesjj
Copy link
Contributor Author

hughesjj commented Aug 31, 2023

@gsanchezgavier

Random value identifying the Redis server (to be used by Sentinel and Cluster)

Checking the latest semantic conventions, it does seem redundant to set service.instance.id to endpoint:port or similar given endpoint and port are already required.

I could be convinced to set service.instance.id to the run_id. It's an existing problem with ephemeral instances such as lambda etc. If the user wants to explicitly tag a resource, they can always define it themselves in their receiver configuration via an attributes processor etc, which imho is a more consistent way.

Semantically, I would argue that "instance" means "instantiation". If you terminate + restart the application or hardware, you've birthed a new "instance".

So, provided it's consistent as expected, yes, I'm all for run_id, but let's double check possible implications.

@gsanchezgavier
Copy link
Contributor

Checking the latest semantic conventions, it does seem redundant to set service.instance.id to endpoint:port or similar given endpoint and port are already required.

Make sense, I didn't see they already were there.

If the user wants to explicitly tag a resource, they can always define it themselves in their receiver configuration via an attributes processor etc, which imho is a more consistent way.

We (in NewRelic) are thinking of recommending this as a way to identify the Instance but it becomes a bit harder to set when you want to have more than one Redis Receiver in the config, since it requires creating different processors and assigning them to each receiver in different pipelines.

Regarding run_id as service.instance.id I fear that it could increase cardinality without providing much value rather than understanding if the Redis instance has been restarted, which perhaps could be solved with a restart count metric.

Copy link
Contributor

github-actions bot commented Nov 1, 2023

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.

@github-actions github-actions bot added the Stale label Nov 1, 2023
mx-psi pushed a commit that referenced this issue Nov 14, 2023
…ibutes (#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:** #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 <[email protected]>
RoryCrispin pushed a commit to ClickHouse/opentelemetry-collector-contrib that referenced this issue 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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request receiver/redis Redis related issues Stale
Projects
None yet
Development

No branches or pull requests

3 participants